From 7cd4711e3170d5913955ef3c3b3a2a6fd37058f9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 28 Jun 2017 18:41:57 +0200 Subject: [PATCH 01/11] Revert "libnm: refactor error handling in got_object_manager()" This reverts commit c5370ea71a6c0686d5bffb7039d9053335384f20. --- libnm/nm-client.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 0c519b70ca..774f5f6ba0 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2427,8 +2427,11 @@ got_object_manager (gpointer user_data) init_data->idle_init_id = 0; if (g_cancellable_set_error_if_cancelled (init_data->cancellable, - &error)) - goto out_take_error; + &error)) { + g_simple_async_result_take_error (init_data->result, error); + init_async_complete (init_data); + return G_SOURCE_REMOVE; + } object_manager = g_dbus_object_manager_client_new_for_bus_sync (_nm_dbus_bus_type (), G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_DO_NOT_AUTO_START, @@ -2437,8 +2440,11 @@ got_object_manager (gpointer user_data) proxy_type, NULL, NULL, init_data->cancellable, &error); - if (!object_manager) - goto out_take_error; + if (object_manager == NULL) { + g_simple_async_result_take_error (init_data->result, error); + init_async_complete (init_data); + return G_SOURCE_REMOVE; + } client = init_data->client; priv = NM_CLIENT_GET_PRIVATE (client); @@ -2447,8 +2453,11 @@ got_object_manager (gpointer user_data) name_owner = g_dbus_object_manager_client_get_name_owner (G_DBUS_OBJECT_MANAGER_CLIENT (priv->object_manager)); if (name_owner) { g_free (name_owner); - if (!objects_created (client, priv->object_manager, &error)) - goto out_take_error; + if (!objects_created (client, priv->object_manager, &error)) { + g_simple_async_result_take_error (init_data->result, error); + init_async_complete (init_data); + return G_SOURCE_REMOVE; + } objects = g_dbus_object_manager_get_objects (priv->object_manager); for (iter = objects; iter; iter = iter->next) { @@ -2471,11 +2480,6 @@ got_object_manager (gpointer user_data) g_signal_connect (priv->object_manager, "notify::name-owner", G_CALLBACK (name_owner_changed), client); return G_SOURCE_REMOVE; - -out_take_error: - g_simple_async_result_take_error (init_data->result, error); - init_async_complete (init_data); - return G_SOURCE_REMOVE; } static void From 45d61470981b998865acd5901987a4a4ef8c9320 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 28 Jun 2017 18:42:04 +0200 Subject: [PATCH 02/11] Revert "libnm: don't use async constructor for GDBusObjectManager" Strangely, this breaks systemctl restart NetworkManager nmcli connection up "$NAME" It seems that with this change, libnm misses some events from D-Bus. It looks like there is something seriously broken. Before fixing it, revert the previous state. https://bugzilla.redhat.com/show_bug.cgi?id=1450075 This reverts commit 529d620a59f42e3f06bd4e7de5c817f175803c0d. --- libnm/nm-client.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 774f5f6ba0..1349127718 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2321,7 +2321,6 @@ typedef struct { GCancellable *cancellable; GSimpleAsyncResult *result; int pending_init; - guint idle_init_id; } NMClientInitData; static void @@ -2330,7 +2329,6 @@ init_async_complete (NMClientInitData *init_data) g_simple_async_result_complete (init_data->result); g_object_unref (init_data->result); g_clear_object (&init_data->cancellable); - nm_clear_g_source (&init_data->idle_init_id); g_slice_free (NMClientInitData, init_data); } @@ -2413,8 +2411,8 @@ new_object_manager (GObject *source_object, GAsyncResult *res, gpointer user_dat g_clear_object (&priv->new_object_manager_cancellable); } -static gboolean -got_object_manager (gpointer user_data) +static void +got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) { NMClientInitData *init_data = user_data; NMClient *client; @@ -2424,26 +2422,11 @@ got_object_manager (gpointer user_data) GError *error = NULL; GDBusObjectManager *object_manager; - init_data->idle_init_id = 0; - - if (g_cancellable_set_error_if_cancelled (init_data->cancellable, - &error)) { - g_simple_async_result_take_error (init_data->result, error); - init_async_complete (init_data); - return G_SOURCE_REMOVE; - } - - object_manager = g_dbus_object_manager_client_new_for_bus_sync (_nm_dbus_bus_type (), - G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_DO_NOT_AUTO_START, - "org.freedesktop.NetworkManager", - "/org/freedesktop", - proxy_type, NULL, NULL, - init_data->cancellable, - &error); + object_manager = g_dbus_object_manager_client_new_for_bus_finish (result, &error); if (object_manager == NULL) { g_simple_async_result_take_error (init_data->result, error); init_async_complete (init_data); - return G_SOURCE_REMOVE; + return; } client = init_data->client; @@ -2456,7 +2439,7 @@ got_object_manager (gpointer user_data) if (!objects_created (client, priv->object_manager, &error)) { g_simple_async_result_take_error (init_data->result, error); init_async_complete (init_data); - return G_SOURCE_REMOVE; + return; } objects = g_dbus_object_manager_get_objects (priv->object_manager); @@ -2479,7 +2462,6 @@ got_object_manager (gpointer user_data) g_signal_connect (priv->object_manager, "notify::name-owner", G_CALLBACK (name_owner_changed), client); - return G_SOURCE_REMOVE; } static void @@ -2497,7 +2479,14 @@ prepare_object_manager (NMClient *client, user_data, init_async); g_simple_async_result_set_op_res_gboolean (init_data->result, TRUE); - init_data->idle_init_id = g_idle_add (got_object_manager, init_data); + g_dbus_object_manager_client_new_for_bus (_nm_dbus_bus_type (), + G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_DO_NOT_AUTO_START, + "org.freedesktop.NetworkManager", + "/org/freedesktop", + proxy_type, NULL, NULL, + init_data->cancellable, + got_object_manager, + init_data); } static void From 880a7061f29153ce9662175668c2b1439739e9c8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Jun 2017 14:13:16 +0200 Subject: [PATCH 03/11] libnm: clear NMClientPrivate.object_manager in name_owner_changed() Don't leave dangling pointers. --- libnm/nm-client.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 1349127718..d1dfff4651 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2497,10 +2497,12 @@ name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) GDBusObjectManager *object_manager = G_DBUS_OBJECT_MANAGER (object); gchar *name_owner; + nm_assert (object_manager == priv->object_manager); + name_owner = g_dbus_object_manager_client_get_name_owner (G_DBUS_OBJECT_MANAGER_CLIENT (object)); if (name_owner) { g_free (name_owner); - g_object_unref (object_manager); + g_clear_object (&priv->object_manager); if (priv->new_object_manager_cancellable) g_cancellable_cancel (priv->new_object_manager_cancellable); priv->new_object_manager_cancellable = g_cancellable_new (); From df6e2e2e784c945f7cb5ab186e897db6e5a05b40 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Jun 2017 14:20:38 +0200 Subject: [PATCH 04/11] libnm: fix getting self pointer in NMClient's new_object_manager() --- libnm/nm-client.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index d1dfff4651..a2930a54c3 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2405,7 +2405,8 @@ unhook_om (NMClient *self) static void new_object_manager (GObject *source_object, GAsyncResult *res, gpointer user_data) { - NMClientPrivate *priv = NM_CLIENT_GET_PRIVATE (source_object); + NMClient *self = NM_CLIENT (user_data); + NMClientPrivate *priv = NM_CLIENT_GET_PRIVATE (self); g_object_notify (G_OBJECT (user_data), NM_CLIENT_NM_RUNNING); g_clear_object (&priv->new_object_manager_cancellable); @@ -2507,7 +2508,7 @@ name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) g_cancellable_cancel (priv->new_object_manager_cancellable); priv->new_object_manager_cancellable = g_cancellable_new (); prepare_object_manager (self, priv->new_object_manager_cancellable, - new_object_manager, user_data); + new_object_manager, self); } else { g_signal_handlers_disconnect_by_func (object_manager, object_added, self); unhook_om (self); From e0ddf64522069e51025588897e6a6c7ab0dde181 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Jun 2017 14:22:19 +0200 Subject: [PATCH 05/11] libnm: fix leaking cancellable in NMClient's name_owner_changed() --- libnm/nm-client.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index a2930a54c3..8999f07bab 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2504,8 +2504,7 @@ name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) if (name_owner) { g_free (name_owner); g_clear_object (&priv->object_manager); - if (priv->new_object_manager_cancellable) - g_cancellable_cancel (priv->new_object_manager_cancellable); + nm_clear_g_cancellable (&priv->new_object_manager_cancellable); priv->new_object_manager_cancellable = g_cancellable_new (); prepare_object_manager (self, priv->new_object_manager_cancellable, new_object_manager, self); From d5efcc1115a2ff86009cc01751a4e8a009a7e470 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Jun 2017 14:29:51 +0200 Subject: [PATCH 06/11] libnm: clear cancellable in new_object_manager() before emitting signal Emitting signals may have side-effects. Just clear the cancellable first, it is handled for good. --- libnm/nm-client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 8999f07bab..ca11a92074 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2408,8 +2408,8 @@ new_object_manager (GObject *source_object, GAsyncResult *res, gpointer user_dat NMClient *self = NM_CLIENT (user_data); NMClientPrivate *priv = NM_CLIENT_GET_PRIVATE (self); - g_object_notify (G_OBJECT (user_data), NM_CLIENT_NM_RUNNING); g_clear_object (&priv->new_object_manager_cancellable); + g_object_notify (G_OBJECT (user_data), NM_CLIENT_NM_RUNNING); } static void From efac9ecadd52b00dddd43eea20a78ac506aacb25 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Jun 2017 15:17:31 +0200 Subject: [PATCH 07/11] libnm/trivial: move code --- libnm/nm-client.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index ca11a92074..e4073dfd32 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -91,6 +91,13 @@ G_DEFINE_TYPE_WITH_CODE (NMClient, nm_client, G_TYPE_OBJECT, #define NM_CLIENT_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NM_TYPE_CLIENT, NMClientPrivate)) +typedef struct { + NMClient *client; + GCancellable *cancellable; + GSimpleAsyncResult *result; + int pending_init; +} NMClientInitData; + typedef struct { NMManager *manager; NMRemoteSettings *settings; @@ -2316,13 +2323,6 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error) /* Asynchronous initialization. */ -typedef struct { - NMClient *client; - GCancellable *cancellable; - GSimpleAsyncResult *result; - int pending_init; -} NMClientInitData; - static void init_async_complete (NMClientInitData *init_data) { From 958ae36c179b1438adafbfbefbb16478663ef52d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Jun 2017 15:20:19 +0200 Subject: [PATCH 08/11] libnm: assert in async_inited_obj_nm() for existing pending_init count --- libnm/nm-client.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index e4073dfd32..847c6c8dea 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2338,14 +2338,14 @@ async_inited_obj_nm (GObject *object, GAsyncResult *result, gpointer user_data) NMClientInitData *init_data = user_data; GError *error = NULL; + nm_assert (init_data && init_data->pending_init > 0); + if (!g_async_initable_init_finish (G_ASYNC_INITABLE (object), result, &error)) g_simple_async_result_take_error (init_data->result, error); - if (init_data) { - init_data->pending_init--; - if (init_data->pending_init == 0) - init_async_complete (init_data); - } + init_data->pending_init--; + if (init_data->pending_init == 0) + init_async_complete (init_data); } static void From 4f0a621d436df95467de82b0eb2ec5475e7d5081 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Jun 2017 15:32:23 +0200 Subject: [PATCH 09/11] libnm: fix leaking init_data in got_object_manager() Only happens if there are no objects, which would be very unusual. --- libnm/nm-client.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 847c6c8dea..d9ea5cc95c 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2457,8 +2457,9 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) async_inited_obj_nm, init_data); } g_list_free_full (objects, g_object_unref); + } - } else + if (init_data->pending_init == 0) init_async_complete (init_data); g_signal_connect (priv->object_manager, "notify::name-owner", From a73e73eae140ba88843e7a3dce6dfed03119c699 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Jun 2017 15:34:34 +0200 Subject: [PATCH 10/11] libnm: move check for pending_init to init_async_complete() --- libnm/nm-client.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index d9ea5cc95c..ee9418ce60 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2326,6 +2326,8 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error) static void init_async_complete (NMClientInitData *init_data) { + if (init_data->pending_init > 0) + return; g_simple_async_result_complete (init_data->result); g_object_unref (init_data->result); g_clear_object (&init_data->cancellable); @@ -2344,8 +2346,7 @@ async_inited_obj_nm (GObject *object, GAsyncResult *result, gpointer user_data) g_simple_async_result_take_error (init_data->result, error); init_data->pending_init--; - if (init_data->pending_init == 0) - init_async_complete (init_data); + init_async_complete (init_data); } static void @@ -2459,8 +2460,7 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) g_list_free_full (objects, g_object_unref); } - if (init_data->pending_init == 0) - init_async_complete (init_data); + init_async_complete (init_data); g_signal_connect (priv->object_manager, "notify::name-owner", G_CALLBACK (name_owner_changed), client); From 196bae36bb6028632ecf8c55fbd4e9a4af35a398 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 21 Jun 2017 15:57:07 +0200 Subject: [PATCH 11/11] libnm: refactor name-owner check for object-manager --- libnm/nm-client.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index ee9418ce60..38b69100f9 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2274,13 +2274,23 @@ objects_created (NMClient *client, GDBusObjectManager *object_manager, GError ** static void name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data); +static gboolean +_om_has_name_owner (GDBusObjectManager *object_manager) +{ + gs_free char *name_owner = NULL; + + nm_assert (G_IS_DBUS_OBJECT_MANAGER_CLIENT (object_manager)); + + name_owner = g_dbus_object_manager_client_get_name_owner (G_DBUS_OBJECT_MANAGER_CLIENT (object_manager)); + return !!name_owner; +} + static gboolean init_sync (GInitable *initable, GCancellable *cancellable, GError **error) { NMClient *client = NM_CLIENT (initable); NMClientPrivate *priv = NM_CLIENT_GET_PRIVATE (client); GList *objects, *iter; - gchar *name_owner; priv->object_manager = g_dbus_object_manager_client_new_for_bus_sync (_nm_dbus_bus_type (), G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_DO_NOT_AUTO_START, @@ -2292,9 +2302,7 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error) if (!priv->object_manager) return FALSE; - name_owner = g_dbus_object_manager_client_get_name_owner (G_DBUS_OBJECT_MANAGER_CLIENT (priv->object_manager)); - if (name_owner) { - g_free (name_owner); + if (_om_has_name_owner (priv->object_manager)) { if (!objects_created (client, priv->object_manager, error)) return FALSE; @@ -2420,7 +2428,6 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) NMClient *client; NMClientPrivate *priv; GList *objects, *iter; - gchar *name_owner; GError *error = NULL; GDBusObjectManager *object_manager; @@ -2435,9 +2442,7 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) priv = NM_CLIENT_GET_PRIVATE (client); priv->object_manager = object_manager; - name_owner = g_dbus_object_manager_client_get_name_owner (G_DBUS_OBJECT_MANAGER_CLIENT (priv->object_manager)); - if (name_owner) { - g_free (name_owner); + if (_om_has_name_owner (priv->object_manager)) { if (!objects_created (client, priv->object_manager, &error)) { g_simple_async_result_take_error (init_data->result, error); init_async_complete (init_data); @@ -2497,13 +2502,10 @@ name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) NMClient *self = user_data; NMClientPrivate *priv = NM_CLIENT_GET_PRIVATE (self); GDBusObjectManager *object_manager = G_DBUS_OBJECT_MANAGER (object); - gchar *name_owner; nm_assert (object_manager == priv->object_manager); - name_owner = g_dbus_object_manager_client_get_name_owner (G_DBUS_OBJECT_MANAGER_CLIENT (object)); - if (name_owner) { - g_free (name_owner); + if (_om_has_name_owner (object_manager)) { g_clear_object (&priv->object_manager); nm_clear_g_cancellable (&priv->new_object_manager_cancellable); priv->new_object_manager_cancellable = g_cancellable_new ();