GNOME Bugzilla – Bug 744598
[review] dcbw/wifi-gdbus: port supplicant and wifi code to GDBus
Last modified: 2015-03-03 21:02:36 UTC
Port the supplicant and wifi code to GDBus.
>> supplicant: convert NMSupplicantManager to GDBus on_proxy_acquired: nm_supplicant_manager_init() aquires the proxy async (good). When the instance gets disposed before on_proxy_acquired() returns, the async call gets cancelled (good). only: priv->proxy = g_dbus_proxy_new_for_bus_finish (result, &error); if (!priv->proxy) { + if (!g_error_matches (local, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { nm_log_warn (LOGD_SUPPLICANT, "Failed to acquire wpa_supplicant proxy: Wi-Fi and 802.1x will not be available (%s)", error ? error->message : "unknown"); } g_clear_error (&error); also, I think the cancelling happens async too, right? So you better don't touch @self before we know that the invocation was not cancelled: + NMSupplicantManager *self = NM_SUPPLICANT_MANAGER (user_data); + NMSupplicantManagerPrivate *priv = NM_SUPPLICANT_MANAGER_GET_PRIVATE (self); interface_removed_cb(): do you need such a function? Can you not just provide no callback? If yes, it seems this would better be an utility function nm_utils_dbus_callback_ignore_all()
(In reply to Thomas Haller from comment #1) > interface_removed_cb(): > > do you need such a function? Can you not just provide no callback? No and yes. You can always pass NULL for a GAsyncReadyCallback.
Repushed with two fixups, the first fixes Thomas' comments, and the second ensures that for the NMSupplicantInterface code that 'self' and 'priv' are never accessed before we know the call was cancelled or not.
>> fixup! supplicant: convert NMSupplicantManager to GDBus did you omit this on purpose? if (!proxy) { if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { nm_log_warn (LOGD_SUPPLICANT, "Failed to acquire wpa_suppl... error ? error->message : "unknown"); } g_clear_error (&error); return; } or alternative: gs_free_error GError *error = NULL; proxy = g_dbus_proxy_new_for_bus_finish (result, &error); if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; if (!proxy) { ... otherwise LGTM
(In reply to Thomas Haller from comment #4) > >> fixup! supplicant: convert NMSupplicantManager to GDBus > > did you omit this on purpose? > > if (!proxy) { > if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { > nm_log_warn (LOGD_SUPPLICANT, "Failed to acquire wpa_suppl... > error ? error->message : "unknown"); > } > g_clear_error (&error); > return; > } We dont' care about cancellation here, because we just return in any error case. So there's no reason to treat cancelled separately.
Repushed with one additional fixup (the second "fixup! supplicant: convert interface/config to GDBus") which fixes a use-after-free and also fixes sending blobs to the supplicant.
> supplicant: convert NMSupplicantManager to GDBus >+ nm_log_warn (LOGD_SUPPLICANT, "Failed to acquire wpa_supplicant proxy: Wi-Fi and 802.1x will not be available (%s)", >+ error ? error->message : "unknown"); Tying into the nm-applet libnm bug; this is the other GError anti-pattern I'd like to get rid of. If a function that returns a GError returns a value indicating that it failed, then error *IS* set. You don't have to check it. >+ value = g_dbus_proxy_get_cached_property (priv->proxy, "Capabilities"); The return value of g_dbus_proxy_get_cached_property() needs to be unreffed when you're done with it. >+ array = g_variant_get_strv (value, NULL); >... >+ g_free (array); FWIW, I found g_variant_get_strv()'s memory-allocation semantics to be kinda bizarre and unexpected, so I always used g_variant_dup_strv() everywhere. YMMV. >+ if (value && g_variant_is_of_type (value, G_VARIANT_TYPE_STRING_ARRAY)) { >+ array = g_variant_get_strv (value, NULL); >+ for (iter = array; iter && *iter; iter++) { You don't need to check if iter is NULL. I think dbus-glib was inconsistent about this, but gdbus/gvariant is not: NULL is not a valid value of any D-Bus type, so assuming you have typechecked correctly and don't hit a return-val-if-fail, no g_variant_get_TYPE() function ever returns NULL (well, except g_variant_get_maybe(), but D-Bus doesn't use maybe types). >+ reply = g_dbus_proxy_call_finish (proxy, result, NULL); >+ if (reply) { >+ /* Reply ignored */ >+ g_variant_unref (reply); >+ } GAsyncCallback usage note #2: in addition to the fact that you don't need to pass a GAsyncReadyCallback if you don't want to do anything, you also don't need to call the _finish() function if you don't care about the result. (Async funcs are always supposed to be designed so that if the caller doesn't call the _finish function, then the return value they would have gotten will get freed when the GAsyncResult gets freed.) > supplicant: clean up NMSupplicantInterface::dispose() Well, I wasn't going to say anything, but the fact that you split this into a separate commit for NMSupplicantInterface makes me wish you had done it for NMSupplicantManager too... :) > supplicant: convert interface/config to GDBus >+ while (g_variant_iter_next (&viter, "{&sv}", (gpointer) &key, (gpointer) &v)) { >+ if (MATCH_PROPERTY (key, "KeyMgmt", v, G_VARIANT_TYPE_STRING_ARRAY)) { Given that you aren't doing anything with unmatched keys, this can be simplified by doing lookups rather than iterating, and then you get the typechecking for free too: if (g_variant_lookup (security, "KeyMgmt", "^as", &items)) { ... } if (g_variant_lookup (security, "Pairwise", "^as", &items)) { ... } if (g_variant_lookup (security, "Group", "&s", &tmp)) { ... } (and don't check if items/tmp are NULL; they aren't). Also, possibly use _nm_utils_string_in_list(). Likewise in nm_ap_new_from_properties() >+bss_props_changed_cb (GDBusProxy *proxy, >+ GVariant *changed_properties, >+ GStrv invalidated_properties, "char **invalidated_properties" >+ props = g_variant_builder_end (&builder); >+ g_signal_emit (self, signals[NEW_BSS], 0, >+ g_dbus_proxy_get_object_path (proxy), >+ props); Floating GVariants in signal emissions are tricky IIRC. It would be best to ref_sink it before the emission and unref it after. (Also, you should be using G_TYPE_VARIANT, not G_TYPE_POINTER in the signal definition.) >+ bss_proxy = g_object_new (G_TYPE_DBUS_PROXY, >... >+ g_hash_table_insert (priv->bss_proxies, (char *) object_path, bss_proxy); Nothing ever actually looks at the values in priv->bss_proxies, so you could store just the path rather than mapping paths to proxies, and then you could just use g_dbus_proxy_new_for_bus() >+ if (variant || _dbus_error_has_name (error, "fi.w1.wpa_supplicant1.InvalidArgs")) The other possibility here is that you could have registered an error domain for wpa_supplicant, and then you could just use g_error_matches(). Not saying you necessarily should, just that that's another possibility. >+ g_variant_get (variant, "(s)", &data); > if (data && strstr (data, "ProbeRequest")) don't need to check that data is non-NULL, and also, you should change it to use (&s) so you don't need to free it later. >+ g_dbus_proxy_call (priv->wpas_proxy, >+ "CreateInterface", >+ g_variant_new ("(@a{sv})", g_variant_builder_end (&props)), or just g_variant_new ("(a{sv})", &props); (Likewise in a few other places)
(In reply to Dan Winship from comment #7) > > supplicant: convert NMSupplicantManager to GDBus > > >+ nm_log_warn (LOGD_SUPPLICANT, "Failed to acquire wpa_supplicant proxy: Wi-Fi and 802.1x will not be available (%s)", > >+ error ? error->message : "unknown"); > > Tying into the nm-applet libnm bug; this is the other GError avnti-pattern > I'd like to get rid of. If a function that returns a GError returns a value > indicating that it failed, then error *IS* set. You don't have to check it. Fixed that up in the supplicant manager sources. > >+ value = g_dbus_proxy_get_cached_property (priv->proxy, "Capabilities"); > > The return value of g_dbus_proxy_get_cached_property() needs to be unreffed > when you're done with it. Fixed in a fixup! commit. > >+ array = g_variant_get_strv (value, NULL); > >... > >+ g_free (array); > > FWIW, I found g_variant_get_strv()'s memory-allocation semantics to be kinda > bizarre and unexpected, so I always used g_variant_dup_strv() everywhere. > YMMV. Yeah, I noticed that but just went with the transfer=container pattern to save the memory allocation of all the strings. > >+ if (value && g_variant_is_of_type (value, G_VARIANT_TYPE_STRING_ARRAY)) { > >+ array = g_variant_get_strv (value, NULL); > >+ for (iter = array; iter && *iter; iter++) { > > You don't need to check if iter is NULL. I think dbus-glib was inconsistent > about this, but gdbus/gvariant is not: NULL is not a valid value of any > D-Bus type, so assuming you have typechecked correctly and don't hit a > return-val-if-fail, no g_variant_get_TYPE() function ever returns NULL > (well, except g_variant_get_maybe(), but D-Bus doesn't use maybe types). Fixed up everywhere. > >+ reply = g_dbus_proxy_call_finish (proxy, result, NULL); > >+ if (reply) { > >+ /* Reply ignored */ > >+ g_variant_unref (reply); > >+ } > > GAsyncCallback usage note #2: in addition to the fact that you don't need to > pass a GAsyncReadyCallback if you don't want to do anything, you also don't > need to call the _finish() function if you don't care about the result. > (Async funcs are always supposed to be designed so that if the caller > doesn't call the _finish function, then the return value they would have > gotten will get freed when the GAsyncResult gets freed.) Hmm, I think you were looking at an old branch, I updated it Thu/Fri to remove this in a fixup commit. > > supplicant: clean up NMSupplicantInterface::dispose() > > Well, I wasn't going to say anything, but the fact that you split this into > a separate commit for NMSupplicantInterface makes me wish you had done it > for NMSupplicantManager too... :) Done. > > supplicant: convert interface/config to GDBus > > >+ while (g_variant_iter_next (&viter, "{&sv}", (gpointer) &key, (gpointer) &v)) { > >+ if (MATCH_PROPERTY (key, "KeyMgmt", v, G_VARIANT_TYPE_STRING_ARRAY)) { > > Given that you aren't doing anything with unmatched keys, this can be > simplified by doing lookups rather than iterating, and then you get the > typechecking for free too: > > if (g_variant_lookup (security, "KeyMgmt", "^as", &items)) { > ... > } > > if (g_variant_lookup (security, "Pairwise", "^as", &items)) { > ... > } > > if (g_variant_lookup (security, "Group", "&s", &tmp)) { > ... > } > > (and don't check if items/tmp are NULL; they aren't). Also, possibly use > _nm_utils_string_in_list(). > > Likewise in nm_ap_new_from_properties() Done in a fixup. > >+bss_props_changed_cb (GDBusProxy *proxy, > >+ GVariant *changed_properties, > >+ GStrv invalidated_properties, > > "char **invalidated_properties" Fixed. > >+ props = g_variant_builder_end (&builder); > >+ g_signal_emit (self, signals[NEW_BSS], 0, > >+ g_dbus_proxy_get_object_path (proxy), > >+ props); > > Floating GVariants in signal emissions are tricky IIRC. It would be best to > ref_sink it before the emission and unref it after. (Also, you should be > using G_TYPE_VARIANT, not G_TYPE_POINTER in the signal definition.) Fixed in a fixup. It's unrefed automatically when it falls out of scope due to the gsystem stuff. > >+ bss_proxy = g_object_new (G_TYPE_DBUS_PROXY, > >... > >+ g_hash_table_insert (priv->bss_proxies, (char *) object_path, bss_proxy); > > Nothing ever actually looks at the values in priv->bss_proxies, so you could > store just the path rather than mapping paths to proxies, and then you could > just use g_dbus_proxy_new_for_bus() We have to keep the BSS proxy objects around since we watch for g-properties-changed on them though, and then proxy that out to listeners so that the listeners are insulated from wpa_supplicant specifics. (well, other than property names & types...) > >+ if (variant || _dbus_error_has_name (error, "fi.w1.wpa_supplicant1.InvalidArgs")) > > The other possibility here is that you could have registered an error domain > for wpa_supplicant, and then you could just use g_error_matches(). Not > saying you necessarily should, just that that's another possibility. Yeah, I read up on that but I think for now I'll just leave it as-is. > >+ g_variant_get (variant, "(s)", &data); > > if (data && strstr (data, "ProbeRequest")) > > don't need to check that data is non-NULL, and also, you should change it to > use (&s) so you don't need to free it later. > > >+ g_dbus_proxy_call (priv->wpas_proxy, > >+ "CreateInterface", > >+ g_variant_new ("(@a{sv})", g_variant_builder_end (&props)), > > or just > g_variant_new ("(a{sv})", &props); > > (Likewise in a few other places) Though we still need to call g_variant_builder_clear() after that, so LoC-wise it's a wash. But did this anyway since I think it's a bit clearer (less format-string magic). Repushed, please review again, thanks!
(In reply to Dan Williams from comment #8) > > or just > > g_variant_new ("(a{sv})", &props); > > > > (Likewise in a few other places) > > Though we still need to call g_variant_builder_clear() after that No, you don't. Passing a GVariantBuilder to g_variant_new() calls g_variant_builder_end() on it, which calls g_variant_builder_clear() itself. The only time you need to explicitly call g_variant_builder_clear() is if you start to build something and then run into an error partway through and abort without creating a GVariant out of the builder.
(In reply to Dan Winship from comment #9) > (In reply to Dan Williams from comment #8) > > > or just > > > g_variant_new ("(a{sv})", &props); > > > > > > (Likewise in a few other places) > > > > Though we still need to call g_variant_builder_clear() after that > > No, you don't. Passing a GVariantBuilder to g_variant_new() calls > g_variant_builder_end() on it, which calls g_variant_builder_clear() itself. > > The only time you need to explicitly call g_variant_builder_clear() is if > you start to build something and then run into an error partway through and > abort without creating a GVariant out of the builder. Ok, fixed that up and pushed. I'd looked into glib code here, but got lost in all the g_variant va-based functions. Repushed, please review.
>+ /* priv->ifaces may be modified if availability changes; can't use GHashTableIter */ >+ ifaces = g_hash_table_get_values (priv->ifaces); >+ for (iter = ifaces; iter; iter = iter->next) >+ nm_supplicant_interface_set_supplicant_available (NM_SUPPLICANT_INTERFACE (iter->data), available); "may be modified" how? If interfaces can be removed from the hash table by a call to nm_supplicant_interface_set_supplicant_available(), then this might crash (since the interface would be unreffed when it was removed from the table, but would still be present in ifaces). >+ if (g_variant_lookup (properties, "Mode", "&s", &s)) { >+ if (!g_strcmp0 (s, "infrastructure")) >+ nm_ap_set_mode (ap, NM_802_11_MODE_INFRA); >+ else if (!g_strcmp0 (s, "ad-hoc")) don't need strcmp0; s can't be NULL
(In reply to Dan Winship from comment #11) > >+ /* priv->ifaces may be modified if availability changes; can't use GHashTableIter */ > >+ ifaces = g_hash_table_get_values (priv->ifaces); > >+ for (iter = ifaces; iter; iter = iter->next) > >+ nm_supplicant_interface_set_supplicant_available (NM_SUPPLICANT_INTERFACE (iter->data), available); > > "may be modified" how? If interfaces can be removed from the hash table by a > call to nm_supplicant_interface_set_supplicant_available(), then this might > crash (since the interface would be unreffed when it was removed from the > table, but would still be present in ifaces). When the supplicant interface is set !available, that triggers a state change in the interface, which NMDeviceWifi (and Ethernet) listen for, and they then call nm_supplicant_manager_iface_release(). That removes the interface from priv->ifaces and drops the last reference. None of the WiFi/Ethernet code touches the interface after this (because after nm_supplicant_manager_iface_release() the interface is dead), and luckily nothing in availability_changed() touches it after that either. But I've modified the code and comment to make that more apparent. > >+ if (g_variant_lookup (properties, "Mode", "&s", &s)) { > >+ if (!g_strcmp0 (s, "infrastructure")) > >+ nm_ap_set_mode (ap, NM_802_11_MODE_INFRA); > >+ else if (!g_strcmp0 (s, "ad-hoc")) > > don't need strcmp0; s can't be NULL Fixed. Also another fixup commit to move to g_dbus_variant_lookup() and pass "@" and the variant, instead of g_dbus_variant_lookup_value().
LGTM
19c0de8 merge: replace usage of dbus-glib in supplicant code with GDBus (bgo #744598) 9adbc05 supplicant: remove unused nm-call-store.c/.h 59c8192 supplicant: convert interface/config to GDBus 47fe1b3 supplicant: clean up some whitespace 7ed2d7a supplicant: make NMSupplicantInterface independent of NMSupplicantManager 0e8f5b2 supplicant: clean up NMSupplicantInterface::dispose() 9f5f141 supplicant: convert NMSupplicantManager to GDBus 742b28f supplicant: clean up NMSupplicantManager::dispose()