Hi Leena,
On Thu, Oct 21, 2010 at 06:23:25PM +0530, [email protected] wrote:
> Kindly review the patch for below connman bugs:
> http://bugs.meego.com/show_bug.cgi?id=8363
> http://bugs.meego.com/show_bug.cgi?id=8324
>
> Steps to reproduce: Launch connmand with -P wifi_legacy, connect to a WiFi
> AP, then disconnect/remove, WiFi APs will disappear.
> This bug is also seen randomly even without disconnecting/removing networks
> after a scan is complete.
>
> Analysis:
> When connmand is started without the wifi_legacy plugin, the new wifi plugin
> is loaded. When a scan is initiated by the wifi plugin,
> connman_device_set_scanning() is invoked with scanning set to true. This will
> in turn mark all the current networks as unavailable. Now when scanning is
> over, gsupplicant library will invoke the scan_callback of the wifi plugin.
> But the current BSSs data returned by scan is not obtained and also the
> networks are not updated/marked as available. The wifi plugin's scan_callback
> will now invoke connman_device_set_scanning() with scanning set to false.
> This will in turn cleanup and remove all networks which are not connected and
> unavailable. As a result the connman_networks are removed and the
> `test-connman srrvices` will display an empty list.
>
> Solution:
> On getting a signal_scan_done if the scan is successful, fetching the scan
> BSSs data and updating the existing connman network list will fix this issue.
> The patch does the below:
>
> - Add a new API supplicant_dbus_property_get to gsupplicant library which
> will get the property for the specified method name. This is required to get
> the BSSs data. In future this API can also be used for other properties
> methods.
>
> - scan_bss_data () and scan_network_update () functions to get the BSSs data
> and update existing network list respectively.
>
> - scan_network_update () invokes wifi plugin's network_added() callback for
> existing connman network list which will update the network data and also set
> them back as available.
>
> - Modify wifi plugin's network_added() to look for existing network based on
> the identifier and not path.
>
Very nice, I appreciate your detailed explanations as it makes my reviewer
life easier. Some minor comments on your patch:
> Patch:
> ------
> gsupplicant/dbus.c | 82
> ++++++++++++++++++++++++++++++++++++++++++++++
> gsupplicant/dbus.h | 5 +++
> gsupplicant/supplicant.c | 64 +++++++++++++++++++++++++++++-------
> plugins/wifi.c | 2 +-
> 4 files changed, 140 insertions(+), 13 deletions(-)
>
> diff --git a/gsupplicant/dbus.c b/gsupplicant/dbus.c
> index 23593f4..2d27ff9 100644
> --- a/gsupplicant/dbus.c
> +++ b/gsupplicant/dbus.c
> @@ -182,6 +182,88 @@ int supplicant_dbus_property_get_all(const char *path,
> const char *interface,
> return 0;
> }
>
> +static void property_get_reply(DBusPendingCall *call, void *user_data)
> +{
> + struct property_get_data *data = user_data;
> + DBusMessage *reply;
> + DBusMessageIter iter;
> +
> + reply = dbus_pending_call_steal_reply(call);
> +
> + if (dbus_message_get_type(reply) == DBUS_MESSAGE_TYPE_ERROR)
> + goto done;
> +
> + if (dbus_message_iter_init(reply, &iter) == FALSE)
> + goto done;
> +
> + if (dbus_message_iter_get_arg_type(&iter) == DBUS_TYPE_VARIANT) {
> + DBusMessageIter variant;
> +
> + dbus_message_iter_recurse(&iter, &variant);
> +
> + if (data->function != NULL)
> + data->function(NULL, &variant, data->user_data);
> + }
> +done:
> + dbus_message_unref(reply);
> +
> + dbus_pending_call_unref(call);
> +}
> +
> +int supplicant_dbus_property_get(const char *path, const char *interface,
> + const char *method,
Trailing whitespaces.
> + supplicant_dbus_property_function function,
> + void *user_data)
> +{
> + struct property_get_data *data;
> + DBusMessage *message;
> + DBusPendingCall *call;
> +
> + if (connection == NULL)
> + return -EINVAL;
> +
> + if (path == NULL || interface == NULL || method == NULL)
> + return -EINVAL;
> +
> + data = dbus_malloc0(sizeof(*data));
> + if (data == NULL)
> + return -ENOMEM;
> +
> + message = dbus_message_new_method_call(SUPPLICANT_SERVICE, path,
> + DBUS_INTERFACE_PROPERTIES, "Get");
> + if (message == NULL) {
> + dbus_free(data);
> + return -ENOMEM;
> + }
> +
> + dbus_message_set_auto_start(message, FALSE);
> +
> + dbus_message_append_args(message, DBUS_TYPE_STRING, &interface,
> DBUS_TYPE_STRING, &method, NULL);
> +
This line is over 80 characters.
> + if (dbus_connection_send_with_reply(connection, message,
> + &call, TIMEOUT) == FALSE) {
> + dbus_message_unref(message);
> + dbus_free(data);
> + return -EIO;
> + }
> +
> + if (call == NULL) {
> + dbus_message_unref(message);
> + dbus_free(data);
> + return -EIO;
> + }
> +
> + data->function = function;
> + data->user_data = user_data;
> +
> + dbus_pending_call_set_notify(call, property_get_reply,
> + data, dbus_free);
> +
> + dbus_message_unref(message);
> +
> + return 0;
> +}
> +
> struct property_set_data {
> supplicant_dbus_result_function function;
> void *user_data;
> diff --git a/gsupplicant/dbus.h b/gsupplicant/dbus.h
> index 254b93e..642e8b1 100644
> --- a/gsupplicant/dbus.h
> +++ b/gsupplicant/dbus.h
> @@ -51,6 +51,11 @@ int supplicant_dbus_property_get_all(const char *path,
> const char *interface,
> supplicant_dbus_property_function function,
> void *user_data);
>
> +int supplicant_dbus_property_get(const char *path, const char *interface,
> + const char *method,
> + supplicant_dbus_property_function function,
> + void *user_data);
> +
> int supplicant_dbus_property_set(const char *path, const char *interface,
> const char *key, const char *signature,
> supplicant_dbus_setup_function setup,
> diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
> index 2670c94..a2a188b 100644
> --- a/gsupplicant/supplicant.c
> +++ b/gsupplicant/supplicant.c
> @@ -1300,6 +1300,46 @@ static void interface_property(const char *key,
> DBusMessageIter *iter,
> key, dbus_message_iter_get_arg_type(iter));
> }
>
> +static void scan_network_update(DBusMessageIter *iter, void *user_data)
Trailing whitespace.
> +{
> + GSupplicantInterface *interface = user_data;
> +
Trailing whitspace here.
> + if (iter) {
Here I'd prefer if you would just return if iter is NULL, and then go on with
the code block below.
> + GSupplicantNetwork *network;
> + char *path;
> +
> + dbus_message_iter_get_basic(iter, &path);
> +
> + if (path == NULL)
> + return;
> +
> + if (g_strcmp0(path, "/") == 0)
> + return;
> +
> + /* Update the network details based on scan BSS data */
> + network = g_hash_table_lookup(interface->bss_mapping, path);
> + if (network != NULL)
Trailing whitespace.
> + callback_network_added(network);
> + }
Trailing whitspace.
> +}
> +
> +static void scan_bss_data(const char *key, DBusMessageIter *iter,
> + void *user_data)
> +{
> + GSupplicantInterface *interface = user_data;
> +
Trailing whitespace (There are more trailing whitespaces, please fix all
them).
> + if (iter)
> + supplicant_dbus_array_foreach(iter, scan_network_update,
> interface);
> +
> + if (interface->scan_callback != NULL) {
No need for brackets here.
> + interface->scan_callback(0, interface,
> + interface->scan_data);
You could have this one here in one single line.
> + }
> +
> + interface->scan_callback = NULL;
> + interface->scan_data = NULL;
> +}
> +
> static GSupplicantInterface *interface_alloc(const char *path)
> {
> GSupplicantInterface *interface;
> @@ -1511,18 +1551,19 @@ static void signal_scan_done(const char *path,
> DBusMessageIter *iter)
>
> dbus_message_iter_get_basic(iter, &success);
>
> - if (interface->scan_callback != NULL) {
> - int result = 0;
> -
> - if (success == FALSE)
> - result = -EIO;
> -
> - interface->scan_callback(result, interface,
> - interface->scan_data);
> + /* If scan is successful, get the scanned BSSs and update the network
> + * details accordingly else return -EIO
> + */
> + if (success) {
> + supplicant_dbus_property_get(path, SUPPLICANT_INTERFACE
> ".Interface",
> + "BSSs", scan_bss_data,
> interface);
>
Line is over 80 chars.
Moreover, here as well I'd prefer:
if (success == FALSE) {
if (interface->scan_callback != NULL)
interface->scan_callback(-EIO, interface,
interface->scan_data);
interface->scan_callback = NULL;
interface->scan_data = NULL;
return;
}
supplicant_dbus_property_get();
> + } else {
> + if (interface->scan_callback != NULL)
> + interface->scan_callback(-EIO, interface,
> + interface->scan_data);
> + interface->scan_callback = NULL;
> + interface->scan_data = NULL;
> }
> -
> - interface->scan_callback = NULL;
> - interface->scan_data = NULL;
> }
>
> static void signal_bss_added(const char *path, DBusMessageIter *iter)
> @@ -1769,7 +1810,6 @@ static void interface_get_result(const char *error,
> SUPPLICANT_DBG("");
>
> if (error != NULL) {
> - g_warning("error %s", error);
> err = -EIO;
> goto create;
> }
This chunk is irrelevant to this patch.
> diff --git a/plugins/wifi.c b/plugins/wifi.c
> index 8dafe9f..90acc5e 100644
> --- a/plugins/wifi.c
> +++ b/plugins/wifi.c
> @@ -430,7 +430,7 @@ static void network_added(GSupplicantNetwork
> *supplicant_network)
>
> ssid = g_supplicant_network_get_ssid(supplicant_network, &ssid_len);
>
> - network = connman_device_get_network(wifi->device, path);
> + network = connman_device_get_network(wifi->device, identifier);
This is actually a fix for a separate bug. I applied it to our tree already,
please don't include it with your next patch revision.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
