From b071e9ade7a5921b9e4f55c4d7acb5678293624f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 15 Jun 2017 19:13:53 +0200 Subject: [PATCH 1/4] libnm: complete async result in got_object_manager() also when cancelled Cancelling an operation shall not mean to not invoke the result callback. The result callback is *always* to be invoked. (cherry picked from commit c5a48b7a0b77bcec7a70dfb1b6f2f914998ecac2) --- libnm/nm-client.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 47f1548e24..43af72d57e 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2424,10 +2424,8 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) object_manager = g_dbus_object_manager_client_new_for_bus_finish (result, &error); if (object_manager == NULL) { - if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { - g_simple_async_result_take_error (init_data->result, error); - init_async_complete (init_data); - } + g_simple_async_result_take_error (init_data->result, error); + init_async_complete (init_data); return; } From ed9954c133ccc0c7aa5803af20441949fc40f794 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 15 Jun 2017 19:14:21 +0200 Subject: [PATCH 2/4] libnm: don't use async constructor for GDBusObjectManager The current implementation of GDBusObjectManagerClient implements GAsyncInitableIface, however it simply runs GInitableIface's synchronous init on another thread. I suspect that there are races in the way that is implemented. For one, we see crashes and warnings (rh#1450075, rh#1457769, rh#1457223). Also, it seems very wrong to me, how GDBusObjectManagerClient mixes asynchronous signals (on_control_proxy_g_signal) with synchronously getting all objects (process_get_all_result, GetManagedObjects). I think we should ditch GDBusObjectManager altogether, including the gdbus-codegen skeletons. They add layers of code, for something that should be simple to do directly. For now, just don't do asynchronous initialization on another thread, so we at least avoid this kind of multithreadding issue. This may make the initialization of NMClient a bit slower. (cherry picked from commit 529d620a59f42e3f06bd4e7de5c817f175803c0d) --- libnm/nm-client.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 43af72d57e..dc40f4f27f 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2321,6 +2321,7 @@ typedef struct { GCancellable *cancellable; GSimpleAsyncResult *result; int pending_init; + guint idle_init_id; } NMClientInitData; static void @@ -2329,6 +2330,7 @@ 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); } @@ -2411,8 +2413,8 @@ new_object_manager (GObject *source_object, GAsyncResult *res, gpointer user_dat g_clear_object (&priv->new_object_manager_cancellable); } -static void -got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) +static gboolean +got_object_manager (gpointer user_data) { NMClientInitData *init_data = user_data; NMClient *client; @@ -2422,11 +2424,26 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) GError *error = NULL; GDBusObjectManager *object_manager; - object_manager = g_dbus_object_manager_client_new_for_bus_finish (result, &error); + 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); if (object_manager == NULL) { g_simple_async_result_take_error (init_data->result, error); init_async_complete (init_data); - return; + return G_SOURCE_REMOVE; } client = init_data->client; @@ -2439,7 +2456,7 @@ got_object_manager (GObject *object, GAsyncResult *result, 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; + return G_SOURCE_REMOVE; } objects = g_dbus_object_manager_get_objects (priv->object_manager); @@ -2462,6 +2479,7 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) g_signal_connect (priv->object_manager, "notify::name-owner", G_CALLBACK (name_owner_changed), client); + return G_SOURCE_REMOVE; } static void @@ -2479,14 +2497,7 @@ prepare_object_manager (NMClient *client, user_data, init_async); g_simple_async_result_set_op_res_gboolean (init_data->result, TRUE); - 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); + init_data->idle_init_id = g_idle_add (got_object_manager, init_data); } static void From 7c744c445bc557975685e34c8c44f1b375788748 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 15 Jun 2017 19:15:09 +0200 Subject: [PATCH 3/4] libnm: refactor error handling in got_object_manager() (cherry picked from commit c5370ea71a6c0686d5bffb7039d9053335384f20) --- libnm/nm-client.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index dc40f4f27f..75eaf3d192 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -2427,11 +2427,8 @@ got_object_manager (gpointer user_data) 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; - } + &error)) + goto out_take_error; 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, @@ -2440,11 +2437,8 @@ got_object_manager (gpointer user_data) proxy_type, NULL, NULL, init_data->cancellable, &error); - if (object_manager == NULL) { - g_simple_async_result_take_error (init_data->result, error); - init_async_complete (init_data); - return G_SOURCE_REMOVE; - } + if (!object_manager) + goto out_take_error; client = init_data->client; priv = NM_CLIENT_GET_PRIVATE (client); @@ -2453,11 +2447,8 @@ 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)) { - g_simple_async_result_take_error (init_data->result, error); - init_async_complete (init_data); - return G_SOURCE_REMOVE; - } + if (!objects_created (client, priv->object_manager, &error)) + goto out_take_error; objects = g_dbus_object_manager_get_objects (priv->object_manager); for (iter = objects; iter; iter = iter->next) { @@ -2480,6 +2471,11 @@ 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 5458b36bfc29a25dca110b8af8aed579d8888716 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 15 Jun 2017 21:16:42 +0200 Subject: [PATCH 4/4] libnm: remove property getter for NMObject's "dbus-object" and "dbus-object-manager" These properties are internal and shall not be publicly accessible. Remove the getter. We may later no longer use GDBusObjectManager. It should be an implementation detail, not exposed in the public API of NMObject. (cherry picked from commit 357fd6ba822a23297325d2c87b8f8a6daabcb790) --- libnm/nm-object.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/libnm/nm-object.c b/libnm/nm-object.c index 95346e06e9..6bf1d0636d 100644 --- a/libnm/nm-object.c +++ b/libnm/nm-object.c @@ -1233,12 +1233,6 @@ get_property (GObject *object, guint prop_id, case PROP_DBUS_CONNECTION: g_value_set_object (value, g_dbus_object_manager_client_get_connection (G_DBUS_OBJECT_MANAGER_CLIENT (priv->object_manager))); break; - case PROP_DBUS_OBJECT: - g_value_set_object (value, priv->object); - break; - case PROP_DBUS_OBJECT_MANAGER: - g_value_set_object (value, priv->object_manager); - break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); break; @@ -1335,7 +1329,7 @@ nm_object_class_init (NMObjectClass *nm_object_class) (object_class, PROP_DBUS_OBJECT, g_param_spec_object (NM_OBJECT_DBUS_OBJECT, "", "", G_TYPE_DBUS_OBJECT, - G_PARAM_READWRITE | + G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS)); @@ -1348,7 +1342,7 @@ nm_object_class_init (NMObjectClass *nm_object_class) (object_class, PROP_DBUS_OBJECT_MANAGER, g_param_spec_object (NM_OBJECT_DBUS_OBJECT_MANAGER, "", "", G_TYPE_DBUS_OBJECT_MANAGER, - G_PARAM_READWRITE | + G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS)); }