From d4d00ee09c218afa2802be8ca21e6b67da908b13 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 3 Jun 2017 14:41:29 +0200 Subject: [PATCH 1/2] supplicant/trivial: move code around --- src/supplicant/nm-supplicant-interface.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/supplicant/nm-supplicant-interface.c b/src/supplicant/nm-supplicant-interface.c index ddbc868634..c89429791e 100644 --- a/src/supplicant/nm-supplicant-interface.c +++ b/src/supplicant/nm-supplicant-interface.c @@ -47,6 +47,13 @@ typedef struct { struct _AddNetworkData; +typedef struct { + NMSupplicantInterface *self; + const char *type; + char *bssid; + char *pin; +} WpsEnrollStartData; + typedef struct { NMSupplicantInterface *self; NMSupplicantConfig *cfg; @@ -587,12 +594,7 @@ nm_supplicant_interface_set_pmf_support (NMSupplicantInterface *self, priv->pmf_support = pmf_support; } -typedef struct { - NMSupplicantInterface *self; - const char *type; - char *bssid; - char *pin; -} WpsEnrollStartData; +/*****************************************************************************/ static void wps_enroll_start_data_free (WpsEnrollStartData *data) From 9bfb4f0d09c20b84aac9a406b265328d267e0d8b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 28 Apr 2017 16:22:37 +0200 Subject: [PATCH 2/2] supplicant: chain asynchronous requests for WPS Don't have pending asynchronous requests in parallel, like setting "ProcessCredentials" and "Start", or "Cancel" and "Start". Instead, "Start" is only scheduled after "ProcessCredentials" completed and "ProcessCredentials" is only scheduled after "Cancel" completed. Also, handle the async response of these requests. For one, to achive the chaining mentioned above and to log what happens and possible errors. Upon new enrollment, a previously created GDBusProxy is now reused, where the first operation is to Cancel the previous action. Also, consistently log what is happening. Not doing all of this is less lines of code. It's also simpler, and faster. But in my opinion, it is (usually) better to check and wait for return values, instead of firing off async requests uncontrolled. It allows us to better know where we are and to log about each individual step. This also makes all operations cancellable. Undoubtedly, correctness and handling failures conflicts with simplicity in this case -- or at least: what I think is "correctness" conflicts. --- src/supplicant/nm-supplicant-interface.c | 347 +++++++++++++++++------ 1 file changed, 258 insertions(+), 89 deletions(-) diff --git a/src/supplicant/nm-supplicant-interface.c b/src/supplicant/nm-supplicant-interface.c index c89429791e..0ebe345837 100644 --- a/src/supplicant/nm-supplicant-interface.c +++ b/src/supplicant/nm-supplicant-interface.c @@ -49,10 +49,13 @@ struct _AddNetworkData; typedef struct { NMSupplicantInterface *self; - const char *type; + char *type; char *bssid; char *pin; -} WpsEnrollStartData; + GDBusProxy *proxy; + GCancellable *cancellable; + bool is_cancelling; +} WpsData; typedef struct { NMSupplicantInterface *self; @@ -116,8 +119,7 @@ typedef struct { GDBusProxy * iface_proxy; GCancellable * other_cancellable; - GDBusProxy * wps_proxy; /* We only have a WPS proxy when the enrollment was initiated */ - GCancellable * wps_cancellable; /* This can cancel the initiation, not the enrollment */ + WpsData *wps_data; AssocData * assoc_data; @@ -597,60 +599,86 @@ nm_supplicant_interface_set_pmf_support (NMSupplicantInterface *self, /*****************************************************************************/ static void -wps_enroll_start_data_free (WpsEnrollStartData *data) +_wps_data_free (WpsData *data) { + g_free (data->type); g_free (data->pin); g_free (data->bssid); - g_slice_free (WpsEnrollStartData, data); + g_clear_object (&data->cancellable); + if (data->proxy && data->self) + g_signal_handlers_disconnect_by_data (data->proxy, data->self); + g_clear_object (&data->proxy); + g_slice_free (WpsData, data); } static void -wpas_iface_wps_credentials (GDBusProxy *proxy, - GVariant *props, - gpointer user_data) +_wps_credentials_changed_cb (GDBusProxy *proxy, + GVariant *props, + gpointer user_data) { NMSupplicantInterface *self = NM_SUPPLICANT_INTERFACE (user_data); + _LOGT ("wps: new credentials"); g_signal_emit (self, signals[WPS_CREDENTIALS], 0, props); } static void -on_wps_proxy_acquired (GDBusProxy *proxy, GAsyncResult *result, gpointer user_data) +_wps_handle_start_cb (GObject *source_object, + GAsyncResult *res, + gpointer user_data) { - WpsEnrollStartData *data = user_data; - NMSupplicantInterface *self = data->self; - NMSupplicantInterfacePrivate *priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self); - GVariantBuilder start_args; - guint8 bssid_buf[ETH_ALEN]; + NMSupplicantInterface *self; + WpsData *data; + gs_unref_variant GVariant *result = NULL; gs_free_error GError *error = NULL; - priv->wps_proxy = g_dbus_proxy_new_for_bus_finish (result, &error); - if (!priv->wps_proxy) { - if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { - _LOGW ("failed to acquire WPS proxy: (%s)", error->message); - set_state (self, NM_SUPPLICANT_INTERFACE_STATE_DOWN); - } - wps_enroll_start_data_free (data); + result = g_dbus_proxy_call_finish (G_DBUS_PROXY (source_object), res, &error); + if ( !result + && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; - } - /* Enable Credentials processing. */ - _nm_dbus_signal_connect (priv->wps_proxy, "Credentials", G_VARIANT_TYPE ("(a{sv})"), - G_CALLBACK (wpas_iface_wps_credentials), self); + data = user_data; + self = data->self; - g_dbus_proxy_call (priv->wps_proxy, - "org.freedesktop.DBus.Properties.Set", - g_variant_new ("(ssv)", - WPAS_DBUS_IFACE_INTERFACE_WPS, - "ProcessCredentials", - g_variant_new_boolean (TRUE)), - G_DBUS_CALL_FLAGS_NONE, - -1, - NULL, - NULL, - NULL); + if (result) + _LOGT ("wps: started with success"); + else + _LOGW ("wps: start failed with %s", error->message); + + g_clear_object (&data->cancellable); + nm_clear_g_free (&data->type); + nm_clear_g_free (&data->pin); + nm_clear_g_free (&data->bssid); +} + +static void +_wps_handle_set_pc_cb (GObject *source_object, + GAsyncResult *res, + gpointer user_data) +{ + WpsData *data; + NMSupplicantInterface *self; + gs_unref_variant GVariant *result = NULL; + gs_free_error GError *error = NULL; + GVariantBuilder start_args; + guint8 bssid_buf[ETH_ALEN]; + + result = g_dbus_proxy_call_finish (G_DBUS_PROXY (source_object), res, &error); + if ( !result + && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + return; + + data = user_data; + self = data->self; + + if (result) + _LOGT ("wps: ProcessCredentials successfully set, starting..."); + else + _LOGW ("wps: ProcessCredentials failed to set (%s), starting...", error->message); + + _nm_dbus_signal_connect (data->proxy, "Credentials", G_VARIANT_TYPE ("(a{sv})"), + G_CALLBACK (_wps_credentials_changed_cb), self); - /* Initiate the enrollment. */ g_variant_builder_init (&start_args, G_VARIANT_TYPE_VARDICT); g_variant_builder_add (&start_args, "{sv}", "Role", g_variant_new_string ("enrollee")); g_variant_builder_add (&start_args, "{sv}", "Type", g_variant_new_string (data->type)); @@ -661,24 +689,194 @@ on_wps_proxy_acquired (GDBusProxy *proxy, GAsyncResult *result, gpointer user_da /* The BSSID is in fact not mandatory. If it is not set the supplicant would * enroll with any BSS in range. */ if (!nm_utils_hwaddr_aton (data->bssid, bssid_buf, sizeof (bssid_buf))) - g_return_if_reached (); + nm_assert_not_reached (); g_variant_builder_add (&start_args, "{sv}", "Bssid", g_variant_new_fixed_array (G_VARIANT_TYPE_BYTE, bssid_buf, ETH_ALEN, sizeof (guint8))); - _LOGI ("starting '%s' WPS enrollment for BSSID '%s'", data->type, data->bssid); - } else { - _LOGI ("starting '%s' WPS enrollment", data->type); } - g_dbus_proxy_call (priv->wps_proxy, + g_dbus_proxy_call (data->proxy, "Start", g_variant_new ("(a{sv})", &start_args), G_DBUS_CALL_FLAGS_NONE, -1, + data->cancellable, + _wps_handle_start_cb, + data); +} + +static void +_wps_call_set_pc (WpsData *data) +{ + g_dbus_proxy_call (data->proxy, + "org.freedesktop.DBus.Properties.Set", + g_variant_new ("(ssv)", + WPAS_DBUS_IFACE_INTERFACE_WPS, + "ProcessCredentials", + g_variant_new_boolean (TRUE)), + G_DBUS_CALL_FLAGS_NONE, + -1, + data->cancellable, + _wps_handle_set_pc_cb, + data); +} + +static void +_wps_handle_proxy_cb (GObject *source_object, + GAsyncResult *res, + gpointer user_data) +{ + NMSupplicantInterface *self; + NMSupplicantInterfacePrivate *priv; + WpsData *data; + gs_free_error GError *error = NULL; + GDBusProxy *proxy; + + proxy = g_dbus_proxy_new_for_bus_finish (res, &error); + if ( !proxy + && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + return; + + data = user_data; + self = data->self; + priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self); + + if (!proxy) { + _LOGW ("wps: failure to create D-Bus proxy: %s", error->message); + _wps_data_free (data); + priv->wps_data = NULL; + return; + } + + data->proxy = proxy; + _LOGT ("wps: D-Bus proxy created. set ProcessCredentials..."); + _wps_call_set_pc (data); +} + +static void +_wps_handle_cancel_cb (GObject *source_object, + GAsyncResult *res, + gpointer user_data) +{ + NMSupplicantInterface *self; + NMSupplicantInterfacePrivate *priv; + WpsData *data; + gs_unref_variant GVariant *result = NULL; + gs_free_error GError *error = NULL; + + result = g_dbus_proxy_call_finish (G_DBUS_PROXY (source_object), res, &error); + if ( !result + && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + return; + + data = user_data; + self = data->self; + + if (!self) { + _wps_data_free (data); + if (result) + _LOGT ("wps: cancel completed successfully, after supplicant interface is gone"); + else + _LOGW ("wps: cancel failed (%s), after supplicant interface is gone", error->message); + return; + } + + priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self); + + data->is_cancelling = FALSE; + + if (!data->type) { + priv->wps_data = NULL; + _wps_data_free (data); + if (result) + _LOGT ("wps: cancel completed successfully"); + else + _LOGW ("wps: cancel failed (%s)", error->message); + return; + } + + if (result) + _LOGT ("wps: cancel completed successfully, setting ProcessCredentials now..."); + else + _LOGW ("wps: cancel failed (%s), setting ProcessCredentials now...", error->message); + _wps_call_set_pc (data); +} + +static void +_wps_start (NMSupplicantInterface *self, + const char *type, + const char *bssid, + const char *pin) +{ + NMSupplicantInterfacePrivate *priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self); + WpsData *data = priv->wps_data; + + if (type) + _LOGI ("wps: type %s start...", type); + + if (!data) { + if (!type) + return; + + data = g_slice_new0 (WpsData); + data->self = self; + data->type = g_strdup (type); + data->bssid = g_strdup (bssid); + data->pin = g_strdup (pin); + data->cancellable = g_cancellable_new (); + + priv->wps_data = data; + + _LOGT ("wps: create D-Bus proxy..."); + + g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, + WPAS_DBUS_SERVICE, + priv->object_path, + WPAS_DBUS_IFACE_INTERFACE_WPS, + data->cancellable, + _wps_handle_proxy_cb, + data); + return; + } + + g_free (data->type); + g_free (data->bssid); + g_free (data->pin); + data->type = g_strdup (type); + data->bssid = g_strdup (bssid); + data->pin = g_strdup (pin); + + if (!data->proxy) { + if (!type) { + nm_clear_g_cancellable (&data->cancellable); + priv->wps_data = NULL; + _wps_data_free (data); + + _LOGT ("wps: abort creation of D-Bus proxy"); + } else + _LOGT ("wps: new enrollment. Wait for D-Bus proxy..."); + return; + } + + if (data->is_cancelling) + return; + + _LOGT ("wps: cancel previous enrollment..."); + + data->is_cancelling = TRUE; + if (!data->cancellable) + data->cancellable = g_cancellable_new (); + g_signal_handlers_disconnect_by_data (data->proxy, self); + g_dbus_proxy_call (data->proxy, + "Cancel", NULL, - NULL, - NULL); - wps_enroll_start_data_free (data); + G_DBUS_CALL_FLAGS_NONE, + -1, + data->cancellable, + _wps_handle_cancel_cb, + data); } void @@ -687,54 +885,17 @@ nm_supplicant_interface_enroll_wps (NMSupplicantInterface *self, const char *bssid, const char *pin) { - NMSupplicantInterfacePrivate *priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self); - WpsEnrollStartData *data; - - data = g_slice_new0 (WpsEnrollStartData); - data->self = self; - data->type = type; - data->bssid = g_strdup (bssid); - data->pin = g_strdup (pin); - - /* Supersede any previous WPS initiations. */ - nm_supplicant_interface_cancel_wps (self); - - priv->wps_cancellable = g_cancellable_new (); - - g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, - NULL, - WPAS_DBUS_SERVICE, - priv->object_path, - WPAS_DBUS_IFACE_INTERFACE_WPS, - priv->wps_cancellable, - (GAsyncReadyCallback) on_wps_proxy_acquired, - data); + _wps_start (self, type, bssid, pin); } void nm_supplicant_interface_cancel_wps (NMSupplicantInterface *self) { - NMSupplicantInterfacePrivate *priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self); - - nm_clear_g_cancellable (&priv->wps_cancellable); - - if (!priv->wps_proxy) - return; - - _LOGD ("cancelling WPS enrollment"); - g_signal_handlers_disconnect_by_data (priv->wps_proxy, self); - g_dbus_proxy_call (priv->wps_proxy, - "Cancel", - NULL, - G_DBUS_CALL_FLAGS_NONE, - -1, - NULL, - NULL, - NULL); - g_clear_object (&priv->wps_proxy); + _wps_start (self, NULL, NULL, NULL); } +/*****************************************************************************/ + static void iface_introspect_cb (GDBusProxy *proxy, GAsyncResult *result, gpointer user_data) { @@ -1777,6 +1938,16 @@ dispose (GObject *object) NMSupplicantInterface *self = NM_SUPPLICANT_INTERFACE (object); NMSupplicantInterfacePrivate *priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self); + nm_supplicant_interface_cancel_wps (self); + if (priv->wps_data) { + /* we shut down, but an asynchronous Cancel request is pending. + * We don't want to cancel it, so mark wps-data that @self is gone. + * This way, _wps_handle_cancel_cb() knows it must no longer touch + * @self */ + priv->wps_data->self = NULL; + priv->wps_data = NULL; + } + if (priv->assoc_data) { gs_free GError *error = NULL; @@ -1794,8 +1965,6 @@ dispose (GObject *object) g_clear_object (&priv->wpas_proxy); g_clear_pointer (&priv->bss_proxies, (GDestroyNotify) g_hash_table_destroy); - nm_supplicant_interface_cancel_wps (self); - g_clear_pointer (&priv->net_path, g_free); g_clear_pointer (&priv->dev, g_free); g_clear_pointer (&priv->object_path, g_free);