GNOME Bugzilla – Bug 696543
Add support for WiFi based geolocation search and other minor fixes
Last modified: 2013-08-19 13:50:48 UTC
Created attachment 239739 [details] [review] patch Add an accuracy field in ipclient which determines if the search would be IP address based or WiFi based.
Created attachment 239743 [details] [review] test-geoip: Fix test function for IP based search The IP address is passed from the main () to the test function and it is that IP address which should be used.
Created attachment 239744 [details] [review] ipclient: Coding style fix
Created attachment 239745 [details] [review] ipclient: Fix memory leak
Created attachment 239746 [details] [review] lib: Add support for WiFi based search Add an accuracy field in ipclient which determines if the search would be IP address based or WiFi based.
Created attachment 239747 [details] [review] test: test the ipclient with other servers
Review of attachment 239743 [details] [review]: Looks good.
Review of attachment 239744 [details] [review]: Looks good.
Review of attachment 239745 [details] [review]: Yep.
Review of attachment 239746 [details] [review]: To sum up my comments: - Passing the APs should be done from the application, so the API will need to change - WiFi results, given the way that they are gathered into Google's database, should be equivalent to a "street" accuracy. ::: configure.ac @@ +61,3 @@ json-glib-1.0 >= 0.14 + libsoup-2.4 + libnm-glib I should have mentioned this before, but I would expect the client to pass this information. Only the test programs should depend on NetworkManager, and we should disable that portion of the tests if NM isn't available. I can take care of the latter part, as that involves more configure.ac hacking than is wise to see :) ::: geocode-glib/geocode-ip-server/geoip-lookup.c @@ +395,2 @@ g_print ("Content-type: text/plain;charset=us-ascii\n\n"); + /* if query string contains wifi field. request The format is: /* line 1 * line 2 * line 3 */ ::: geocode-glib/geocode-ipclient.c @@ +81,3 @@ g_value_get_string (value)); break; + case PROP_ACCURACY: ACCURACY_REQUESTED. Otherwise it might be confusing when we add the accuracy result to the API. @@ +200,3 @@ + */ +static void +populate_ht_with_wifi_aps (GHashTable *ht) That needs to be in the application code.
Review of attachment 239747 [details] [review]: ::: geocode-glib/test-geoip.c @@ +70,3 @@ +{ + char *server = "http://api.hostip.info/get_json.php"; + geolocation_search (NULL, server, GEOCODE_ACCURACY_IP); You'll need to check the return value, and verify some of the results. I would advise to pass a particular IP address to it, to make sure that the results correspond to what we're expecting.
The patches related to bug 696527 is in conflict with how I implemented the support for wifi based search. Implementation of wifi search - Accuracy is made a property of the GeocodeIpclient. One can set it to either GEOCODE_ACCURACY_IP or GEOCODE_ACCURACY_WIFI. And based on the value of accuracy, the client will either send the wifi access point details or only the IP - but not both. I remember the discussion where hadess suggested to send everything to the server. Sending everything to the server in the form of &wifi=<value>&wifi=<value>...&ip=<value>&accuracy=<value> means that the server needs to find out accuracy from the string, then find all the wifi values or the IP value from the string depending on the accuracy. And it could not be done with soup_form_decode() for the function uses a hashtable to get the fields in the query string. It presupposes unique field names which in our case is not happening. It seemed to me that to let the ipclient decide what to send to the server is simpler than parsing the query string for different values at server side. That way the ipclient sends only the wifi aps when higher accuracy is requested. The server just passes the query string it gets to google without doing anything to it.
(In reply to comment #9) > Review of attachment 239746 [details] [review]: > > To sum up my comments: > - Passing the APs should be done from the application, so the API will need to > change Should we pass the wifi aps as property of the GeocodeIpclient object or a member of the object? <snip> ::: geocode-glib/geocode-ipclient.c > @@ +81,3 @@ > g_value_get_string > (value)); > break; > + case PROP_ACCURACY: > > ACCURACY_REQUESTED. > > Otherwise it might be confusing when we add the accuracy result to the API. > Now that the users of the client library have to provide the wifi ap details, do they need to give a ACCURACY_REQUESTED? If they give wifi aps, that means they want more accurate street level information. If they don't, then whatever information the server can find based on the IP will be passed to them. <snip> Copying from the mail that hadess replied - "> The way I implemented is the following - > > 1. accuracy is a property of the client. one can set it to either > GEOCODE_ACCURACY_IP or GEOCODE_ACCURACY_WIFI. And based on the value > of accuracy, the client will either send the wifi access point details > or only the IP - but not both together. We want to avoid making 2 requests when we already have the information to do a single one." Could you please clarify it? To me, the client makes one request only. "The client can probably do something useful (like show a large focus ring around the location) if the accuracy gathered is less than the accuracy requested. Say there's no coverage for Wi-Fi, but you requested "street" level accuracy. You'd get back a result from the IP, that would at least place you inside the city. It's better than nothing." The way I implemented it, if google web service returns error or there's no wifi ap detail in the query string that the GeocodeIpclient sends to the server, then the server falls back to the IP based search. Does it sound good? I am still confused what should the client send to the server? Since we are using GET method, we are using QUERY_STRING. And if we send wifi access point details, accuracy, ip all together, we need to write out own decoding method for the query string. We can not use libsoup. Because it doesn't support multiple values for the same field. e.g. - the query string now becomes - wifi=mac:01-24-7c-bc-51-46%7Cssid:3x2x%7Css:-37&wifi=mac:09-86-3b-31-97-b2%7Cssid:belkin.7b2%7Css:-47&wifi=mac:28-cf-da-ba-be-13%7Cssid:HERESIARCH%20NETWORK%7Css:-49&wifi=mac:2b-cf-da-ba-be-10%7Cssid: ARCH%20GUESTS%7Css:-52&wifi=mac:08-56-3b-2b-e1-a8%7Cssid:belkin.1a8%7Css:-59&wifi=mac:02-1e-64-fd-df-67%7Cssid:Brown%20Cow%7Css:-59&wifi=mac:2a-cf-df-ba-be-10%7Cssid: ARCH%20GUESTS%7Css:-59 soup_form_decode() takes only one value for the field "wifi" since it stores them into a hashtable. Please let me know, what do you think. Thanks for your feedback. Will correct my patch accordingly.
(In reply to comment #12) > (In reply to comment #9) > > Review of attachment 239746 [details] [review] [details]: > > > > To sum up my comments: > > - Passing the APs should be done from the application, so the API will need to > > change > > Should we pass the wifi aps as property of the GeocodeIpclient object or a > member of the object? Given that the object can be reused to do another query, I would pass this as arguments to the search function. > <snip> > ::: geocode-glib/geocode-ipclient.c > > @@ +81,3 @@ > > g_value_get_string > > (value)); > > break; > > + case PROP_ACCURACY: > > > > ACCURACY_REQUESTED. > > > > Otherwise it might be confusing when we add the accuracy result to the API. > > > > Now that the users of the client library have to provide the wifi ap details, > do they need to give a ACCURACY_REQUESTED? If they give wifi aps, that means > they want more accurate street level information. If they don't, then whatever > information the server can find based on the IP will be passed to them. But you want a result. In the case where the IP cannot be found in the database, we still need to give a try at the Wi-Fi lookup. It's going to be more precise than requested, but it means we had an answer at the end. > <snip> > > Copying from the mail that hadess replied - > > "> The way I implemented is the following - > > > > 1. accuracy is a property of the client. one can set it to either > > GEOCODE_ACCURACY_IP or GEOCODE_ACCURACY_WIFI. And based on the value > > of accuracy, the client will either send the wifi access point details > > or only the IP - but not both together. > > We want to avoid making 2 requests when we already have the information > to do a single one." > > Could you please clarify it? To me, the client makes one request only. If one of the requests failed, we don't want the client to need another request to the web server. > "The client can probably do something useful (like show a large focus > ring around the location) if the accuracy gathered is less than the > accuracy requested. > > Say there's no coverage for Wi-Fi, but you requested "street" level > accuracy. You'd get back a result from the IP, that would at least place > you inside the city. It's better than nothing." > > The way I implemented it, if google web service returns error or there's no > wifi ap detail in the query string that the GeocodeIpclient sends to the > server, then the server falls back to the IP based search. Does it sound good? That sounds fine. > I am still confused what should the client send to the server? Since we are > using GET method, we are using QUERY_STRING. And if we send wifi access point > details, accuracy, ip all together, we need to write out own decoding method > for the query string. We can not use libsoup. Because it doesn't support > multiple values for the same field. > e.g. - the query string now becomes - > > wifi=mac:01-24-7c-bc-51-46%7Cssid:3x2x%7Css:-37&wifi=mac:09-86-3b-31-97-b2%7Cssid:belkin.7b2%7Css:-47&wifi=mac:28-cf-da-ba-be-13%7Cssid:HERESIARCH%20NETWORK%7Css:-49&wifi=mac:2b-cf-da-ba-be-10%7Cssid: > ARCH%20GUESTS%7Css:-52&wifi=mac:08-56-3b-2b-e1-a8%7Cssid:belkin.1a8%7Css:-59&wifi=mac:02-1e-64-fd-df-67%7Cssid:Brown%20Cow%7Css:-59&wifi=mac:2a-cf-df-ba-be-10%7Cssid: > ARCH%20GUESTS%7Css:-59 > > soup_form_decode() takes only one value for the field "wifi" since it stores > them into a hashtable. soup_form_decode() and the helper function are around 50 lines of code it seems (libsoup/libsoup/soup-form.c) so it should be pretty straight forward to decode it.
Created attachment 239940 [details] [review] lib: Add support for WiFi based search Add an accuracy field in ipclient which determines if the search would be IP address based or WiFi based.
Created attachment 239941 [details] [review] test: test the ipclient with other servers
Modified the patch for wifi support based on the feedback received and added it here. Now the flow is like this - Client side: 1. The test application collects wifi access point details and pass them to the search function through a hashtable. 2. GeocodeIpclient passes the wifi access points (if it gets them) and IP address (if it gets it) through a query string to the server. Have removed the PROP_ACCURACY_REQUESTED - since it seemed redundant to me now. Server side: 1. The server first checks the query string. If it finds wifi access points, it makes a request to the google web service. 2. If the request fails or the server did not get the wifi access points in the first place, it performs IP based geolocation search using the Maxmind databases. Please let me know, if anything needs to be changed in this flow.
Attachment 239743 [details] pushed as 2764b9f - test-geoip: Fix test function for IP based search Attachment 239744 [details] pushed as 31a7df7 - ipclient: Coding style fix Attachment 239745 [details] pushed as 1773728 - ipclient: Fix memory leak
Review of attachment 239940 [details] [review]: Once you've split the patches, we can discuss the server API in greater details. This is important to do before we write the client API as even though we can change the client API fairly easily, the server API will be hard to change once deployed. ::: configure.ac @@ +61,3 @@ json-glib-1.0 >= 0.14 + libsoup-2.4 + libnm-glib The check needs to be separate, just for the test application, otherwise the library will be linked to NetworkManager as well. ::: geocode-glib/geocode-ip-server/geoip-lookup.c @@ +6,3 @@ #include <json-glib/json-glib.h> #include <libsoup/soup.h> +#include <string.h> The code in geoip-lookup.c should be in a separate patch. @@ +446,3 @@ + } + if (strcmp (name, "wifi") == 0) + g_hash_table_insert (form_data_set, name, value); You keep inserting values with the same associated name, which means that you're replacing the previous item. That's not going to work. Google's API uses the AP name, its MAC address and signal strength. You could extract the AP name, and use that as the key. Though I'm not very happy with the client passing the data in the exact same format as Google expects. It makes things easy right now, but if we ever switch providers, we'll have problems. One word of caution as well. AP names might not always be printable characters. So you need to make sure you don't use g_strdup() on them. @@ +447,3 @@ + if (strcmp (name, "wifi") == 0) + g_hash_table_insert (form_data_set, name, value); + else { if you have a brace for the else section of the conditional, you should have it for the first. Don't do: if (bleh) foo(); else { bar(); baz(); } but: if (bleh) { foo(); } else { bar(); baz(); } @@ +448,3 @@ + g_hash_table_insert (form_data_set, name, value); + else { + if (strcmp (name, "ip") == 0) { In this case, the if could be merged into the else branch: if (name == wifi) { foo(); } else if (name == ip) { .... ::: geocode-glib/geocode-ipclient.c @@ +262,3 @@ * geocode_ipclient_search_async: * @ipclient: a #GeocodeIpclient representing a query + * @ht_wifi: a #GHashTable containing the nearby WiFi access point details please mention what the keys and values are. You can do this using introspection annotations: https://live.gnome.org/GObjectIntrospection/Annotations In this case it would include: (element-type KTYPE VTYPE) ::: geocode-glib/geocode-ipclient.h @@ +30,3 @@ +#define GEOCODE_ACCURACY_IP 0 +#define GEOCODE_ACCURACY_WIFI 1 You can remove this.
Review of attachment 239941 [details] [review]: The code looks fine, but we'll need to wait until Zeeshan works on his patch for compatibility-mode.
Created attachment 240773 [details] [review] server: Add support for WiFi based search The server first checks the query string. If it finds wifi access points, it makes a request to Google web service. If the request fails or the server did not get the wifi access points in the QUERY_STRING, it performs IP based geolocation search using the Maxmind databases. It can find the IP address from either the QUERY_STRING or various proxy variables
Created attachment 240776 [details] [review] server: Add support for WiFi based search The server first checks the query string. If it finds wifi access points, it makes a request to Google web service. If the request fails or the server did not get the wifi access points in the QUERY_STRING, it performs IP based geolocation search using the Maxmind databases. It can find the IP address from either the QUERY_STRING or various proxy variables
(In reply to comment #18) <snip> > You keep inserting values with the same associated name, which means that > you're replacing the previous item. That's not going to work. I am using the following declaration for the hashtable which hashes on the pointers. g_hash_table_new_full (g_direct_hash, g_direct_equal, g_free, NULL); Therefore, all the wifi aps in the near by area can be stored in it. > Google's API uses the AP name, its MAC address and signal strength. You could > extract the AP name, and use that as the key. > Though I'm not very happy with the client passing the data in the exact same > format as Google expects. It makes things easy right now, but if we ever switch > providers, we'll have problems. > Google uses them in the format - wifi=mac:<value>|ssid<AP name>|ss<signal strength>&wifi=mac:<value>|ssid<AP name>|ss<signal strength>&wifi=mac:<value>|ssid<AP name>|ss<signal strength>.... And the client sends the query string exactly in this format. But a better implementation should be, as you said, to store it in such a manner so that it can be manipulated later as well. <snip>
Created attachment 240778 [details] [review] server: Add support for WiFi based search The server first checks the query string. If it finds wifi access points, it makes a request to Google web service. If the request fails or the server did not get the wifi access points in the QUERY_STRING, it performs IP based geolocation search using the Maxmind databases. It can find the IP address from either the QUERY_STRING or various proxy variables
Review of attachment 240778 [details] [review]: ::: geocode-glib/geocode-ip-server/geoip-lookup.c @@ +331,3 @@ + SoupMessage *msg; + char *final_uri; + char *base_uri = "https://maps.googleapis.com/maps/api/browserlocation/json?browser=firefox&sensor=true"; Do we really need to lie about browser here?
(In reply to comment #24) > Review of attachment 240778 [details] [review]: > > ::: geocode-glib/geocode-ip-server/geoip-lookup.c > @@ +331,3 @@ > + SoupMessage *msg; > + char *final_uri; > + char *base_uri = > "https://maps.googleapis.com/maps/api/browserlocation/json?browser=firefox&sensor=true"; > > Do we really need to lie about browser here? Yes! Otherwise Google gives error. This web service is only meant for Mozilla to use and I could not find any official documentation of this service. We are planning to get permission from Google to use this service for the geoip server,
Review of attachment 240778 [details] [review]: Mainly problems with comments. ::: geocode-glib/geocode-ip-server/geoip-lookup.c @@ +274,3 @@ +/* parse_json_for_wifi function parses the JSON output from + * google web service and converts it into the JSON format + * used by geocode-glib library. On failure, it returns false. FALSE. @@ +276,3 @@ + * used by geocode-glib library. On failure, it returns false. + * It doesn't print any error message since the IP based search + * is performed if wifi based search fails. Wi-Fi @@ +317,3 @@ + + /* TODO: What should be the text of the attribution text + */ Close the comment on the same line. @@ +331,3 @@ + SoupMessage *msg; + char *final_uri; + char *base_uri = "https://maps.googleapis.com/maps/api/browserlocation/json?browser=firefox&sensor=true"; This should be a #define. @@ +346,3 @@ + +/* Taken from libsoup/soup-form.c + */ Ditto, one-line comment. @@ +373,3 @@ + +/** + * decode_query_string () returns a hashtable of wifi ap Wi-Fi. AP. @@ +374,3 @@ +/** + * decode_query_string () returns a hashtable of wifi ap + * details. And fills the ipaddress if there's any ip no need to stop the sentence here. IP. @@ +380,3 @@ + * be sent to google. That way we can reduce the intermediate + * creation of the hashtable and then encoding the the + * query string for google web service Google. @@ +395,3 @@ + for (i = 0; pairs[i]; i++) { + name = pairs[i]; + eq = strchr (name, '='); if (!eq) { g_free (pairs[i]); continue; } @@ +433,2 @@ g_print ("Content-type: text/plain;charset=us-ascii\n\n"); + /* if query string contains wifi field. request If Wi-Fi @@ +433,3 @@ g_print ("Content-type: text/plain;charset=us-ascii\n\n"); + /* if query string contains wifi field. request + * information from google webservice. otherwise Google. @@ +434,3 @@ + /* if query string contains wifi field. request + * information from google webservice. otherwise + * use maxmind database for IP based search. IP MaxMind. @@ +440,3 @@ + if (data != NULL) { + ht = decode_query_string (data, &ipaddress); + if (ht) { decode_query_string() always returns a hashtable. @@ +464,2 @@ if (!ipaddress) + return 1; This looks unrelated, and could be done in a separate patch. @@ +468,2 @@ g_free (ipaddress); + Unrelated whitespace change.
Comment on attachment 240778 [details] [review] server: Add support for WiFi based search Committed the server patch for now. We'll need to have documentation written out for the server before anything else gets committed so that we can iterate on it.
Do you mean gtk-doc annotations for the functions in the server?
(In reply to comment #28) > Do you mean gtk-doc annotations for the functions in the server? I meant API documentation for the server's web interface (possible parameters, implied parameters, possible errors, output format used, etc.).
(In reply to comment #29) > (In reply to comment #28) > > Do you mean gtk-doc annotations for the functions in the server? > > I meant API documentation for the server's web interface (possible parameters, > implied parameters, possible errors, output format used, etc.). where shall we add them?
(In reply to comment #30) <snip> > where shall we add them? As mentioned on IRC, a text file would probably do for now, though we'll want to integrate this in the gtk-doc output later on.
Created attachment 243002 [details] [review] server: API documention for server's web interface
Review of attachment 243002 [details] [review]: Can you rename it to API-Documentation.txt and put it in the same location as the geoip-server? Can you also wrap the text lines (not the ones with code) at just below 80 characters? ::: geoip-server-guide @@ +7,3 @@ +You can install it using the script geocode-glib/geocode-ip-server/tests/launch-web-server.sh. + +$cd geocode-glib/geocode-ip-server/tests Add a space after "$" @@ +11,3 @@ +$./launch-web-server.sh stop - to stop the server + +The script will install the server at your http://localhost:12345. It doesn't install it, it will set it up. @@ +52,3 @@ +6. What are the possible error scenarios and error messages? + +The server gives the following errors in the following cases - Document where the "0", "1", "2" numbers come from ("They correspond to error codes from the ... enum in http://git.gnome.org/browse/...")
Created attachment 244222 [details] [review] server: API documention for server's web interface
Made the changes. (In reply to comment #33) > Review of attachment 243002 [details] [review]: > > Can you rename it to API-Documentation.txt and put it in the same location as > the geoip-server? > Can you also wrap the text lines (not the ones with code) at just below 80 > characters? > > ::: geoip-server-guide > @@ +7,3 @@ > +You can install it using the script > geocode-glib/geocode-ip-server/tests/launch-web-server.sh. > + > +$cd geocode-glib/geocode-ip-server/tests > > Add a space after "$" > > @@ +11,3 @@ > +$./launch-web-server.sh stop - to stop the server > + > +The script will install the server at your http://localhost:12345. > > It doesn't install it, it will set it up. > > @@ +52,3 @@ > +6. What are the possible error scenarios and error messages? > + > +The server gives the following errors in the following cases - > > Document where the "0", "1", "2" numbers come from > ("They correspond to error codes from the ... enum in > http://git.gnome.org/browse/...")
Comment on attachment 244222 [details] [review] server: API documention for server's web interface Attachment 244222 [details] pushed as 4592595 - server: API documention for server's web interface
do we need this for 3.10 to make geolocation a useful feature ?
(In reply to comment #37) > do we need this for 3.10 to make geolocation a useful feature ? Not really. For settings-daemon, and clocks city-level accuracy is good enough. Don't know if weather is even intending to use geoclue in time for 3.10 but even if it is, not really a big deal IMO to just show weather for the city (rather than the region). Would have been cool for Maps but city-level is good enough for preview release I guess. Besides, we decided to not go with Google's geolocation API and even if we hadn't, someone needs to ask permission from Google and it would take a while before Google could give us its verdict. Oh and btw, I'm in the process of moving geolocation code to geoclue (where it belongs) so these patches will have to be reworked for geoclue.
Moved to Geoclue bz as geolocation code is now there: https://bugs.freedesktop.org/show_bug.cgi?id=68278