From d95717cdbe7d9770d1a0d1620a89f638d116bc78 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 Apr 2018 08:45:55 +0200 Subject: [PATCH 01/40] auth-chain: remove unused error argument --- src/nm-auth-utils.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 5d92f36d6a..93aee45008 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -40,7 +40,6 @@ struct NMAuthChain { GDBusMethodInvocation *context; NMAuthSubject *subject; - GError *error; NMAuthChainResultFunc done_func; gpointer user_data; @@ -219,7 +218,7 @@ auth_chain_finish (gpointer user_data) /* Ensure we stay alive across the callback */ self->refcount++; - self->done_func (self, self->error, self->context, self->user_data); + self->done_func (self, NULL, self->context, self->user_data); nm_auth_chain_unref (self); return FALSE; } @@ -423,7 +422,6 @@ nm_auth_chain_unref (NMAuthChain *self) while ((call = c_list_first_entry (&self->auth_call_lst_head, AuthCall, auth_call_lst))) auth_call_free (call); - g_clear_error (&self->error); nm_clear_pointer (&self->data, g_hash_table_destroy); g_slice_free (NMAuthChain, self); From 67e06924e1aeba92a7987ba888dcee9eda11d9f4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 20:36:35 +0200 Subject: [PATCH 02/40] core/trivial: add code comment to NMActiveConnection's get_property() --- src/nm-active-connection.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index 88074e2302..a24e946d31 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -1176,6 +1176,15 @@ get_property (GObject *object, guint prop_id, NMDevice *master_device = NULL; switch (prop_id) { + + /* note that while priv->settings_connection might not be set initially, + * it will be set before the object is exported on D-Bus. Hence, + * nobody is calling these property getters before the object is + * exported, at which point we will have a valid settings-connection. + * + * Therefore, intentionally not check whether priv->settings_connection + * is set, to get an assertion failure if somebody tries to access the + * getters at the wrong time. */ case PROP_CONNECTION: g_value_set_string (value, nm_connection_get_path (NM_CONNECTION (priv->settings_connection))); break; @@ -1188,6 +1197,7 @@ get_property (GObject *object, guint prop_id, case PROP_TYPE: g_value_set_string (value, nm_connection_get_connection_type (NM_CONNECTION (priv->settings_connection))); break; + case PROP_SPECIFIC_OBJECT: g_value_set_string (value, priv->specific_object ? priv->specific_object : "/"); break; From 76df7a3088ba44817bef601465b891f364ef3aec Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 30 Mar 2018 20:10:34 +0200 Subject: [PATCH 03/40] shared: add nm_dbus_utils_get_paths_for_clist() util --- src/nm-dbus-utils.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/nm-dbus-utils.h | 7 +++++++ 2 files changed, 47 insertions(+) diff --git a/src/nm-dbus-utils.c b/src/nm-dbus-utils.c index 2464ff79f7..7f7fb27873 100644 --- a/src/nm-dbus-utils.c +++ b/src/nm-dbus-utils.c @@ -152,4 +152,44 @@ nm_dbus_utils_g_value_set_object_path_from_hash (GValue *value, g_value_take_boxed (value, strv); } +const char ** +nm_dbus_utils_get_paths_for_clist (const CList *lst_head, + gssize lst_len, + guint member_offset, + gboolean expect_all_exported) +{ + const CList *iter; + const char **strv; + const char *path; + gsize i, n; + + nm_assert (lst_head); + + if (lst_len < 0) + n = c_list_length (lst_head); + else { + n = lst_len; + nm_assert (n == c_list_length (lst_head)); + } + + i = 0; + strv = g_new (const char *, n + 1); + c_list_for_each (iter, lst_head) { + NMDBusObject *obj = (NMDBusObject *) (((const char *) iter) - member_offset); + + path = nm_dbus_object_get_path (obj); + if (!path) { + nm_assert (expect_all_exported); + continue; + } + + nm_assert (i < n); + strv[i++] = path; + } + nm_assert (i <= n); + strv[i] = NULL; + + return strv; +} + /*****************************************************************************/ diff --git a/src/nm-dbus-utils.h b/src/nm-dbus-utils.h index 1d9c92ec36..62945cf14e 100644 --- a/src/nm-dbus-utils.h +++ b/src/nm-dbus-utils.h @@ -164,6 +164,13 @@ GVariant *nm_dbus_utils_get_property (GObject *obj, /*****************************************************************************/ +struct CList; + +const char **nm_dbus_utils_get_paths_for_clist (const struct CList *lst_head, + gssize lst_len, + guint member_offset, + gboolean expect_all_exported); + void nm_dbus_utils_g_value_set_object_path (GValue *value, gpointer object); void nm_dbus_utils_g_value_set_object_path_from_hash (GValue *value, From 8c56c47bba8f1d8d064ac3699308005805a6f6bd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 10:18:57 +0200 Subject: [PATCH 04/40] settings: handle default-wired-connection in connection-removed signal Don't subscribe twice to the same signal. The more subscribers a signal has, the more confusing it gets what is happening. We can handle also the default-wired-connection in the regular connection-removed signal. Note how connection_removed() is registered with g_signal_connect_after(), but that is fine. There are few subscribers to this signal (that don't do anything that interferes here). Especially, since all other subscribers subscribe with the same priority (hence, are unordered). So, moving this task explicitly to after, does not change any ordering guarantee -- in fact, it ensures an ordering that was undefined previously. Anyway, it doesn't matter. --- src/settings/nm-settings.c | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index ade117de5d..4a50d3b9a5 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -166,6 +166,11 @@ static void connection_ready_changed (NMSettingsConnection *conn, GParamSpec *pspec, gpointer user_data); +static void default_wired_clear_tag (NMSettings *self, + NMDevice *device, + NMSettingsConnection *connection, + gboolean add_to_no_auto_default); + /*****************************************************************************/ static void @@ -862,11 +867,21 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) NMSettings *self = NM_SETTINGS (user_data); NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); const char *cpath = nm_connection_get_path (NM_CONNECTION (connection)); + NMDevice *device; if (!g_hash_table_lookup (priv->connections, cpath)) g_return_if_reached (); g_object_ref (connection); + /* When the default wired connection is removed (either deleted or saved to + * a new persistent connection by a plugin), write the MAC address of the + * wired device to the config file and don't create a new default wired + * connection for that device again. + */ + device = g_object_get_qdata (G_OBJECT (connection), _default_wired_device_quark ()); + if (device) + default_wired_clear_tag (self, device, connection, TRUE); + /* Disconnect signal handlers, as plugins might still keep references * to the connection (and thus the signal handlers would still be live) * even after NMSettings has dropped all its references. @@ -1677,26 +1692,6 @@ have_connection_for_device (NMSettings *self, NMDevice *device) return FALSE; } -static void default_wired_clear_tag (NMSettings *self, - NMDevice *device, - NMSettingsConnection *connection, - gboolean add_to_no_auto_default); - -static void -default_wired_connection_removed_cb (NMSettingsConnection *connection, NMSettings *self) -{ - NMDevice *device; - - /* When the default wired connection is removed (either deleted or saved to - * a new persistent connection by a plugin), write the MAC address of the - * wired device to the config file and don't create a new default wired - * connection for that device again. - */ - device = g_object_get_qdata (G_OBJECT (connection), _default_wired_device_quark ()); - if (device) - default_wired_clear_tag (self, device, connection, TRUE); -} - static void default_wired_connection_updated_by_user_cb (NMSettingsConnection *connection, gboolean by_user, NMSettings *self) { @@ -1729,7 +1724,6 @@ default_wired_clear_tag (NMSettings *self, g_object_set_qdata (G_OBJECT (connection), _default_wired_device_quark (), NULL); g_object_set_qdata (G_OBJECT (device), _default_wired_connection_quark (), NULL); - g_signal_handlers_disconnect_by_func (connection, G_CALLBACK (default_wired_connection_removed_cb), self); g_signal_handlers_disconnect_by_func (connection, G_CALLBACK (default_wired_connection_updated_by_user_cb), self); if (add_to_no_auto_default) @@ -1781,8 +1775,6 @@ device_realized (NMDevice *device, GParamSpec *pspec, NMSettings *self) g_signal_connect (added, NM_SETTINGS_CONNECTION_UPDATED_INTERNAL, G_CALLBACK (default_wired_connection_updated_by_user_cb), self); - g_signal_connect (added, NM_SETTINGS_CONNECTION_REMOVED, - G_CALLBACK (default_wired_connection_removed_cb), self); _LOGI ("(%s): created default wired connection '%s'", nm_device_get_iface (device), From 213323a6a7a50ed11608cd536833d74c9e9e399e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 10:30:49 +0200 Subject: [PATCH 05/40] settings: fix unrefing setting is there are no plugins loaded This wasn't a problem, because load_plugins() can only fails if the settings plugins fail to load, which can only happen if you have a broken installation. --- src/settings/nm-settings.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 4a50d3b9a5..bf53b62a07 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1847,10 +1847,8 @@ nm_settings_start (NMSettings *self, GError **error) /* Load the plugins; fail if a plugin is not found. */ plugins = nm_config_data_get_plugins (nm_config_get_data_orig (priv->config), TRUE); - if (!load_plugins (self, (const char **) plugins, error)) { - g_object_unref (self); + if (!load_plugins (self, (const char **) plugins, error)) return FALSE; - } load_connections (self); check_startup_complete (self); From f41279ca1dc14faae433d4c4492b7e2cc49aefaf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 10:39:43 +0200 Subject: [PATCH 06/40] settings: use cleanup attribute to keep connection alive during connection_removed() --- src/settings/nm-settings.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index bf53b62a07..c0a474c297 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -868,10 +868,12 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); const char *cpath = nm_connection_get_path (NM_CONNECTION (connection)); NMDevice *device; + _nm_unused gs_unref_object NMSettingsConnection *connection_keep_alive = NULL; if (!g_hash_table_lookup (priv->connections, cpath)) g_return_if_reached (); - g_object_ref (connection); + + connection_keep_alive = g_object_ref (connection); /* When the default wired connection is removed (either deleted or saved to * a new persistent connection by a plugin), write the MAC address of the @@ -906,8 +908,6 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) nm_dbus_object_unexport (NM_DBUS_OBJECT (connection)); check_startup_complete (self); - - g_object_unref (connection); } #define NM_DBUS_SERVICE_OPENCONNECT "org.freedesktop.NetworkManager.openconnect" From 24c3eacac6a6bb9b0574fa6d33b4083b6f55a90f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 10:50:34 +0200 Subject: [PATCH 07/40] settings/trivial: add code comments --- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c index e66ecfdccc..bc0ad448af 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c @@ -326,7 +326,10 @@ update_connection (SettingsPluginIfcfg *self, if (new_unmanaged || new_unrecognized) { if (!old_unmanaged && !old_unrecognized) { + /* ref connection first, because we put it into priv->connections below. + * Emitting signal-removed might otherwise delete it. */ g_object_ref (connection_by_uuid); + /* Unexport the connection by telling the settings service it's * been removed. */ @@ -340,7 +343,7 @@ update_connection (SettingsPluginIfcfg *self, */ g_hash_table_insert (priv->connections, g_strdup (nm_connection_get_uuid (NM_CONNECTION (connection_by_uuid))), - connection_by_uuid); + connection_by_uuid/*<< took reference above*/); } } else { if (old_unmanaged /* && !new_unmanaged */) { @@ -372,7 +375,9 @@ update_connection (SettingsPluginIfcfg *self, _LOGI ("add connection "NM_IFCFG_CONNECTION_LOG_FMT, NM_IFCFG_CONNECTION_LOG_ARG (connection_new)); else _LOGI ("new connection "NM_IFCFG_CONNECTION_LOG_FMT, NM_IFCFG_CONNECTION_LOG_ARG (connection_new)); - g_hash_table_insert (priv->connections, g_strdup (uuid), connection_new); + g_hash_table_insert (priv->connections, + g_strdup (uuid), + connection_new /* take reference */); g_signal_connect (connection_new, NM_SETTINGS_CONNECTION_REMOVED, G_CALLBACK (connection_removed_cb), From fdbf696f22894304e6f4313edaa57464cfdc5c95 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 10:57:51 +0200 Subject: [PATCH 08/40] settings: clear connection path when unexporting from D-Bus At that point, the path becomes meaningless. Clear it. --- src/settings/nm-settings.c | 4 +++- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c | 3 --- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index c0a474c297..d335850a92 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -904,8 +904,10 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) /* Re-emit for listeners like NMPolicy */ _notify (self, PROP_CONNECTIONS); - if (nm_dbus_object_is_exported (NM_DBUS_OBJECT (connection))) + if (nm_dbus_object_is_exported (NM_DBUS_OBJECT (connection))) { + nm_connection_set_path (NM_CONNECTION (connection), NULL); nm_dbus_object_unexport (NM_DBUS_OBJECT (connection)); + } check_startup_complete (self); } diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c index bc0ad448af..5a8778ba2f 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c @@ -334,9 +334,6 @@ update_connection (SettingsPluginIfcfg *self, * been removed. */ nm_settings_connection_signal_remove (NM_SETTINGS_CONNECTION (connection_by_uuid)); - /* Remove the path so that claim_connection() doesn't complain later when - * interface gets managed and connection is re-added. */ - nm_connection_set_path (NM_CONNECTION (connection_by_uuid), NULL); /* signal_remove() will end up removing the connection from our hash, * so add it back now. From efe5cf79c0eb61b86958f5cc58c7361121d0eede Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 11:09:11 +0200 Subject: [PATCH 09/40] core: simplify NMActiveConnection.get_property to not create temporary GPtrArrray Fixes: c050fb7cd2160f0b74ba8a0760e717e3fe329066 --- src/nm-active-connection.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index a24e946d31..33da0aa131 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -1172,7 +1172,7 @@ get_property (GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) { NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE ((NMActiveConnection *) object); - GPtrArray *devices; + char **strv; NMDevice *master_device = NULL; switch (prop_id) { @@ -1202,11 +1202,10 @@ get_property (GObject *object, guint prop_id, g_value_set_string (value, priv->specific_object ? priv->specific_object : "/"); break; case PROP_DEVICES: - devices = g_ptr_array_sized_new (2); + strv = g_new0 (char *, 2); if (priv->device && priv->state < NM_ACTIVE_CONNECTION_STATE_DEACTIVATED) - g_ptr_array_add (devices, g_strdup (nm_dbus_object_get_path (NM_DBUS_OBJECT (priv->device)))); - g_ptr_array_add (devices, NULL); - g_value_take_boxed (value, (char **) g_ptr_array_free (devices, FALSE)); + strv[0] = g_strdup (nm_dbus_object_get_path (NM_DBUS_OBJECT (priv->device))); + g_value_take_boxed (value, strv); break; case PROP_STATE: if (priv->state_set) From 9efa7c7220aeb0e37ec9131b8815575d10164aa1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 11:40:27 +0200 Subject: [PATCH 10/40] core: use nm_dbus_object_get_path() instead of nm_connection_get_path() Essentially, nm_connection_get_path() mirros nm_dbus_object_get_path(). However, when cloning a simple-connection, the path also gets cloned. I think this field doesn't belong to NMConnection in the first place, because NMConnection is not a D-Bus object. NMSettingsConnection (in core) and NMRemoteConnection (in libnm) is. Don't use the misleading alias, but use nm_dbus_object_get_path() directly. --- src/devices/nm-device.c | 2 +- src/nm-active-connection.c | 2 +- src/nm-dispatcher.c | 2 +- src/nm-manager.c | 4 ++-- src/settings/nm-settings-connection.c | 10 ++++----- src/settings/nm-settings.c | 22 +++++++++---------- .../plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c | 2 +- 7 files changed, 21 insertions(+), 23 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index c0540534e0..ee1aaee821 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -15257,7 +15257,7 @@ get_property (GObject *object, guint prop_id, array = g_ptr_array_sized_new (g_hash_table_size (priv->available_connections)); g_hash_table_iter_init (&iter, priv->available_connections); while (g_hash_table_iter_next (&iter, (gpointer) &connection, NULL)) - g_ptr_array_add (array, g_strdup (nm_connection_get_path (connection))); + g_ptr_array_add (array, g_strdup (nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)))); g_ptr_array_add (array, NULL); g_value_take_boxed (value, (char **) g_ptr_array_free (array, FALSE)); break; diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index 33da0aa131..e78caaf28f 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -1186,7 +1186,7 @@ get_property (GObject *object, guint prop_id, * is set, to get an assertion failure if somebody tries to access the * getters at the wrong time. */ case PROP_CONNECTION: - g_value_set_string (value, nm_connection_get_path (NM_CONNECTION (priv->settings_connection))); + g_value_set_string (value, nm_dbus_object_get_path (NM_DBUS_OBJECT (priv->settings_connection))); break; case PROP_ID: g_value_set_string (value, nm_connection_get_id (NM_CONNECTION (priv->settings_connection))); diff --git a/src/nm-dispatcher.c b/src/nm-dispatcher.c index 2f88d468eb..235134c872 100644 --- a/src/nm-dispatcher.c +++ b/src/nm-dispatcher.c @@ -579,7 +579,7 @@ _dispatcher_call (NMDispatcherAction action, const char *connection_path; const char *filename; - connection_path = nm_connection_get_path (NM_CONNECTION (settings_connection)); + connection_path = nm_dbus_object_get_path (NM_DBUS_OBJECT (settings_connection)); if (connection_path) { g_variant_builder_add (&connection_props, "{sv}", NMD_CONNECTION_PROPS_PATH, diff --git a/src/nm-manager.c b/src/nm-manager.c index c8fad9dbb7..ed1aace333 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2392,7 +2392,7 @@ recheck_assume_connection (NMManager *self, if (!active) { _LOGW (LOGD_DEVICE, "assume: assumed connection %s failed to activate: %s", - nm_connection_get_path (NM_CONNECTION (connection)), + nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)), error->message); g_error_free (error); @@ -4517,7 +4517,7 @@ activation_add_done (NMSettings *settings, g_dbus_method_invocation_return_value ( context, g_variant_new ("(oo)", - nm_connection_get_path (NM_CONNECTION (new_connection)), + nm_dbus_object_get_path (NM_DBUS_OBJECT (new_connection)), nm_dbus_object_get_path (NM_DBUS_OBJECT (active)))); nm_audit_log_connection_op (NM_AUDIT_OP_CONN_ADD_ACTIVATE, nm_active_connection_get_settings_connection (active), diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 713395420e..3270a3de16 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -581,7 +581,7 @@ _update_prepare (NMSettingsConnection *self, if (!nm_connection_normalize (new_connection, NULL, NULL, error)) return FALSE; - if ( nm_connection_get_path (NM_CONNECTION (self)) + if ( nm_dbus_object_get_path (NM_DBUS_OBJECT (self)) && g_strcmp0 (nm_settings_connection_get_uuid (self), nm_connection_get_uuid (new_connection)) != 0) { /* Updating the UUID is not allowed once the path is exported. */ g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, @@ -798,7 +798,7 @@ nm_settings_connection_delete (NMSettingsConnection *self, for_agents = nm_simple_connection_new_clone (NM_CONNECTION (self)); nm_connection_clear_secrets (for_agents); nm_agent_manager_delete_secrets (priv->agent_mgr, - nm_connection_get_path (NM_CONNECTION (self)), + nm_dbus_object_get_path (NM_DBUS_OBJECT (self)), for_agents); g_object_unref (for_agents); @@ -1295,7 +1295,7 @@ nm_settings_connection_get_secrets (NMSettingsConnection *self, priv->last_secret_agent_version_id = nm_agent_manager_get_agent_version_id (priv->agent_mgr); call_id_a = nm_agent_manager_get_secrets (priv->agent_mgr, - nm_connection_get_path (NM_CONNECTION (self)), + nm_dbus_object_get_path (NM_DBUS_OBJECT (self)), NM_CONNECTION (self), subject, existing_secrets, @@ -1789,7 +1789,7 @@ update_auth_cb (NMSettingsConnection *self, secrets_filter_cb, GUINT_TO_POINTER (NM_SETTING_SECRET_FLAG_AGENT_OWNED)); nm_agent_manager_save_secrets (info->agent_mgr, - nm_connection_get_path (NM_CONNECTION (self)), + nm_dbus_object_get_path (NM_DBUS_OBJECT (self)), for_agent, info->subject); } @@ -2210,7 +2210,7 @@ dbus_clear_secrets_auth_cb (NMSettingsConnection *self, /* Tell agents to remove secrets for this connection */ nm_agent_manager_delete_secrets (priv->agent_mgr, - nm_connection_get_path (NM_CONNECTION (self)), + nm_dbus_object_get_path (NM_DBUS_OBJECT (self)), NM_CONNECTION (self)); nm_settings_connection_update (self, diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index d335850a92..a6c78c8998 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -365,7 +365,7 @@ impl_settings_get_connection_by_uuid (NMDBusObject *obj, g_dbus_method_invocation_return_value (invocation, g_variant_new ("(o)", - nm_connection_get_path (NM_CONNECTION (connection)))); + nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)))); return; error: @@ -856,7 +856,7 @@ _emit_connection_removed (NMSettings *self, &interface_info_settings, &signal_info_connection_removed, "(o)", - nm_connection_get_path (NM_CONNECTION (connection))); + nm_dbus_object_get_path (NM_DBUS_OBJECT (connection))); g_signal_emit (self, signals[CONNECTION_REMOVED], 0, connection); } @@ -866,7 +866,7 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) { NMSettings *self = NM_SETTINGS (user_data); NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); - const char *cpath = nm_connection_get_path (NM_CONNECTION (connection)); + const char *cpath = nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)); NMDevice *device; _nm_unused gs_unref_object NMSettingsConnection *connection_keep_alive = NULL; @@ -963,7 +963,7 @@ claim_connection (NMSettings *self, NMSettingsConnection *connection) NMSettingsConnection *existing; g_return_if_fail (NM_IS_SETTINGS_CONNECTION (connection)); - g_return_if_fail (nm_connection_get_path (NM_CONNECTION (connection)) == NULL); + g_return_if_fail (!nm_dbus_object_is_exported (NM_DBUS_OBJECT (connection))); g_hash_table_iter_init (&iter, priv->connections); while (g_hash_table_iter_next (&iter, NULL, &data)) { @@ -1023,13 +1023,11 @@ claim_connection (NMSettings *self, NMSettingsConnection *connection) self); } - /* Export the connection over D-Bus */ - g_warn_if_fail (nm_connection_get_path (NM_CONNECTION (connection)) == NULL); path = nm_dbus_object_export (NM_DBUS_OBJECT (connection)); nm_connection_set_path (NM_CONNECTION (connection), path); g_hash_table_insert (priv->connections, - (gpointer) nm_connection_get_path (NM_CONNECTION (connection)), + (gpointer) nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)), g_object_ref (connection)); _clear_connections_cached_list (&priv->connections_cached_list); @@ -1043,7 +1041,7 @@ claim_connection (NMSettings *self, NMSettingsConnection *connection) &interface_info_settings, &signal_info_new_connection, "(o)", - nm_connection_get_path (NM_CONNECTION (connection))); + nm_dbus_object_get_path (NM_DBUS_OBJECT (connection))); g_signal_emit (self, signals[CONNECTION_ADDED], 0, connection); _notify (self, PROP_CONNECTIONS); @@ -1169,7 +1167,7 @@ send_agent_owned_secrets (NMSettings *self, secrets_filter_cb, GUINT_TO_POINTER (NM_SETTING_SECRET_FLAG_AGENT_OWNED)); nm_agent_manager_save_secrets (priv->agent_mgr, - nm_connection_get_path (NM_CONNECTION (connection)), + nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)), for_agent, subject); g_object_unref (for_agent); @@ -1382,9 +1380,9 @@ settings_add_connection_add_cb (NMSettings *self, g_dbus_method_invocation_return_gerror (context, error); nm_audit_log_connection_op (NM_AUDIT_OP_CONN_ADD, NULL, FALSE, NULL, subject, error->message); } else { - g_dbus_method_invocation_return_value ( - context, - g_variant_new ("(o)", nm_connection_get_path (NM_CONNECTION (connection)))); + g_dbus_method_invocation_return_value (context, + g_variant_new ("(o)", + nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)))); nm_audit_log_connection_op (NM_AUDIT_OP_CONN_ADD, connection, TRUE, NULL, subject, NULL); } diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c index 5a8778ba2f..2d95b535a2 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c @@ -750,7 +750,7 @@ impl_ifcfgrh_get_ifcfg_details (SettingsPluginIfcfg *plugin, return; } - path = nm_connection_get_path (NM_CONNECTION (connection)); + path = nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)); if (!path) { g_dbus_method_invocation_return_error (context, NM_SETTINGS_ERROR, From b0bf9b2b9b5f02a5fb99e8a16076d140a8838773 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 11:47:52 +0200 Subject: [PATCH 11/40] core: explicitly pass D-Bus path to nm_utils_log_connection_diff() No longer rely on nm_connection_get_path() being meaningful in server. It also was wrong. During update, nm_settings_connection_update() would call nm_utils_log_connection_diff (replace_connection, NM_CONNECTION (self), ... where replace_connection has no path set, and nothing was logged. Fix it, by explicitly passing the D-Bus path. Also, because nm-core-utils.c should be independent of nm-dbus-object.h. --- src/nm-core-utils.c | 13 +++++++++---- src/nm-core-utils.h | 7 ++++++- src/settings/nm-settings-connection.c | 6 ++++-- src/settings/nm-settings.c | 3 ++- src/tests/test-general.c | 18 +++++++++--------- 5 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index a9f9255be4..7c36596d0a 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -2234,7 +2234,13 @@ _log_connection_sort_names (LogConnectionSettingData *setting_data, GArray *sort } void -nm_utils_log_connection_diff (NMConnection *connection, NMConnection *diff_base, guint32 level, guint64 domain, const char *name, const char *prefix) +nm_utils_log_connection_diff (NMConnection *connection, + NMConnection *diff_base, + guint32 level, + guint64 domain, + const char *name, + const char *prefix, + const char *dbus_path) { GHashTable *connection_diff = NULL; GArray *sorted_hashes; @@ -2310,7 +2316,6 @@ nm_utils_log_connection_diff (NMConnection *connection, NMConnection *diff_base, if (print_header) { GError *err_verify = NULL; - const char *path = nm_connection_get_path (connection); const char *t1, *t2; t1 = nm_connection_get_connection_type (connection); @@ -2320,12 +2325,12 @@ nm_utils_log_connection_diff (NMConnection *connection, NMConnection *diff_base, prefix, name, connection, G_OBJECT_TYPE_NAME (connection), NM_PRINT_FMT_QUOTE_STRING (t1), diff_base, G_OBJECT_TYPE_NAME (diff_base), NM_PRINT_FMT_QUOTE_STRING (t2), - NM_PRINT_FMT_QUOTED (path, " [", path, "]", "")); + NM_PRINT_FMT_QUOTED (dbus_path, " [", dbus_path, "]", "")); } else { nm_log (level, domain, NULL, NULL, "%sconnection '%s' (%p/%s/%s%s%s):%s%s%s", prefix, name, connection, G_OBJECT_TYPE_NAME (connection), NM_PRINT_FMT_QUOTE_STRING (t1), - NM_PRINT_FMT_QUOTED (path, " [", path, "]", "")); + NM_PRINT_FMT_QUOTED (dbus_path, " [", dbus_path, "]", "")); } print_header = FALSE; diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index 33773ab679..6b3682aa8f 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -231,7 +231,12 @@ const char *nm_utils_new_infiniband_name (char *name, const char *parent_name, i int nm_utils_cmp_connection_by_autoconnect_priority (NMConnection *a, NMConnection *b); -void nm_utils_log_connection_diff (NMConnection *connection, NMConnection *diff_base, guint32 level, guint64 domain, const char *name, const char *prefix); +void nm_utils_log_connection_diff (NMConnection *connection, + NMConnection *diff_base, + guint32 level, guint64 domain, + const char *name, + const char *prefix, + const char *dbus_path); gint64 nm_utils_get_monotonic_timestamp_ns (void); gint64 nm_utils_get_monotonic_timestamp_us (void); diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 3270a3de16..75c4f388ce 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -661,8 +661,10 @@ nm_settings_connection_update (NMSettingsConnection *self, NM_SETTING_COMPARE_FLAG_EXACT)) { gs_unref_object NMConnection *simple = NULL; - if (log_diff_name) - nm_utils_log_connection_diff (replace_connection, NM_CONNECTION (self), LOGL_DEBUG, LOGD_CORE, log_diff_name, "++ "); + if (log_diff_name) { + nm_utils_log_connection_diff (replace_connection, NM_CONNECTION (self), LOGL_DEBUG, LOGD_CORE, log_diff_name, "++ ", + nm_dbus_object_get_path (NM_DBUS_OBJECT (self))); + } /* Make a copy of agent-owned secrets because they won't be present in * the connection returned by plugins, as plugins return only what was diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index a6c78c8998..274630420f 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1031,7 +1031,8 @@ claim_connection (NMSettings *self, NMSettingsConnection *connection) g_object_ref (connection)); _clear_connections_cached_list (&priv->connections_cached_list); - nm_utils_log_connection_diff (NM_CONNECTION (connection), NULL, LOGL_DEBUG, LOGD_CORE, "new connection", "++ "); + nm_utils_log_connection_diff (NM_CONNECTION (connection), NULL, LOGL_DEBUG, LOGD_CORE, "new connection", "++ ", + path); /* Only emit the individual connection-added signal after connections * have been initially loaded. diff --git a/src/tests/test-general.c b/src/tests/test-general.c index 632f2c33cc..38eaf25e91 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -232,13 +232,13 @@ test_nm_utils_log_connection_diff (void) connection = nm_simple_connection_new (); nm_connection_add_setting (connection, nm_setting_connection_new ()); - nm_utils_log_connection_diff (connection, NULL, LOGL_DEBUG, LOGD_CORE, "test1", ">>> "); + nm_utils_log_connection_diff (connection, NULL, LOGL_DEBUG, LOGD_CORE, "test1", ">>> ", NULL); nm_connection_add_setting (connection, nm_setting_wired_new ()); - nm_utils_log_connection_diff (connection, NULL, LOGL_DEBUG, LOGD_CORE, "test2", ">>> "); + nm_utils_log_connection_diff (connection, NULL, LOGL_DEBUG, LOGD_CORE, "test2", ">>> ", NULL); connection2 = nm_simple_connection_new_clone (connection); - nm_utils_log_connection_diff (connection, connection2, LOGL_DEBUG, LOGD_CORE, "test3", ">>> "); + nm_utils_log_connection_diff (connection, connection2, LOGL_DEBUG, LOGD_CORE, "test3", ">>> ", NULL); g_object_set (nm_connection_get_setting_connection (connection), NM_SETTING_CONNECTION_ID, "id", @@ -248,24 +248,24 @@ test_nm_utils_log_connection_diff (void) NM_SETTING_CONNECTION_ID, "id2", NM_SETTING_CONNECTION_MASTER, "master2", NULL); - nm_utils_log_connection_diff (connection, connection2, LOGL_DEBUG, LOGD_CORE, "test4", ">>> "); + nm_utils_log_connection_diff (connection, connection2, LOGL_DEBUG, LOGD_CORE, "test4", ">>> ", NULL); nm_connection_add_setting (connection, nm_setting_802_1x_new ()); - nm_utils_log_connection_diff (connection, connection2, LOGL_DEBUG, LOGD_CORE, "test5", ">>> "); + nm_utils_log_connection_diff (connection, connection2, LOGL_DEBUG, LOGD_CORE, "test5", ">>> ", NULL); g_object_set (nm_connection_get_setting_802_1x (connection), NM_SETTING_802_1X_PASSWORD, "id2", NM_SETTING_802_1X_PASSWORD_FLAGS, NM_SETTING_SECRET_FLAG_NOT_SAVED, NULL); - nm_utils_log_connection_diff (connection, NULL, LOGL_DEBUG, LOGD_CORE, "test6", ">>> "); - nm_utils_log_connection_diff (connection, connection2, LOGL_DEBUG, LOGD_CORE, "test7", ">>> "); - nm_utils_log_connection_diff (connection2, connection, LOGL_DEBUG, LOGD_CORE, "test8", ">>> "); + nm_utils_log_connection_diff (connection, NULL, LOGL_DEBUG, LOGD_CORE, "test6", ">>> ", NULL); + nm_utils_log_connection_diff (connection, connection2, LOGL_DEBUG, LOGD_CORE, "test7", ">>> ", NULL); + nm_utils_log_connection_diff (connection2, connection, LOGL_DEBUG, LOGD_CORE, "test8", ">>> ", NULL); g_clear_object (&connection); g_clear_object (&connection2); connection = nmtst_create_minimal_connection ("id-vpn-1", NULL, NM_SETTING_VPN_SETTING_NAME, NULL); - nm_utils_log_connection_diff (connection, NULL, LOGL_DEBUG, LOGD_CORE, "test-vpn-1", ">>> "); + nm_utils_log_connection_diff (connection, NULL, LOGL_DEBUG, LOGD_CORE, "test-vpn-1", ">>> ", NULL); g_clear_object (&connection); } From 755c8bb30f55e148e6a999f53019c012736a784b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 11:55:45 +0200 Subject: [PATCH 12/40] settings: no longer call nm_connection_set_path() in server It's unused. --- src/settings/nm-settings.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 274630420f..a598871aa3 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -904,10 +904,8 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) /* Re-emit for listeners like NMPolicy */ _notify (self, PROP_CONNECTIONS); - if (nm_dbus_object_is_exported (NM_DBUS_OBJECT (connection))) { - nm_connection_set_path (NM_CONNECTION (connection), NULL); + if (nm_dbus_object_is_exported (NM_DBUS_OBJECT (connection))) nm_dbus_object_unexport (NM_DBUS_OBJECT (connection)); - } check_startup_complete (self); } @@ -1024,10 +1022,9 @@ claim_connection (NMSettings *self, NMSettingsConnection *connection) } path = nm_dbus_object_export (NM_DBUS_OBJECT (connection)); - nm_connection_set_path (NM_CONNECTION (connection), path); g_hash_table_insert (priv->connections, - (gpointer) nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)), + (gpointer) path, g_object_ref (connection)); _clear_connections_cached_list (&priv->connections_cached_list); From de5d07392da488113c956fbf1aca94fa280c3302 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 11:59:03 +0200 Subject: [PATCH 13/40] libnm: optimize nm_simple_connection_new_clone() to not needlessly set the path Server never sets the path, so this is entirely unused server-side. Also NMConnection is a glib interface and stores it's private date in the GObject's data. It's less efficient to look it up. Just avoid it. --- libnm-core/nm-simple-connection.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/libnm-core/nm-simple-connection.c b/libnm-core/nm-simple-connection.c index 11700666f5..f06e1aed4b 100644 --- a/libnm-core/nm-simple-connection.c +++ b/libnm-core/nm-simple-connection.c @@ -113,11 +113,16 @@ NMConnection * nm_simple_connection_new_clone (NMConnection *connection) { NMConnection *clone; + const char *path; g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL); clone = nm_simple_connection_new (); - nm_connection_set_path (clone, nm_connection_get_path (connection)); + + path = nm_connection_get_path (connection); + if (path) + nm_connection_set_path (clone, path); + nm_connection_replace_settings_from_connection (clone, connection); return clone; From a5e9980b344863dfd5b576218d5f6fe1f46635b5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 11:31:31 +0200 Subject: [PATCH 14/40] core: use nm_dbus_utils_g_value_set_object_path_from_hash() --- src/devices/nm-device.c | 12 +++--------- src/settings/nm-settings.c | 11 +++-------- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index ee1aaee821..7f22b67d89 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -15146,9 +15146,6 @@ get_property (GObject *object, guint prop_id, { NMDevice *self = NM_DEVICE (object); NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - GPtrArray *array; - GHashTableIter iter; - NMConnection *connection; GVariantBuilder array_builder; switch (prop_id) { @@ -15254,12 +15251,9 @@ get_property (GObject *object, guint prop_id, g_value_set_uint (value, priv->rfkill_type); break; case PROP_AVAILABLE_CONNECTIONS: - array = g_ptr_array_sized_new (g_hash_table_size (priv->available_connections)); - g_hash_table_iter_init (&iter, priv->available_connections); - while (g_hash_table_iter_next (&iter, (gpointer) &connection, NULL)) - g_ptr_array_add (array, g_strdup (nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)))); - g_ptr_array_add (array, NULL); - g_value_take_boxed (value, (char **) g_ptr_array_free (array, FALSE)); + nm_dbus_utils_g_value_set_object_path_from_hash (value, + priv->available_connections, + TRUE); break; case PROP_PHYSICAL_PORT_ID: g_value_set_string (value, priv->physical_port_id); diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index a598871aa3..dfaf517fc5 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1871,9 +1871,7 @@ get_property (GObject *object, guint prop_id, NMSettings *self = NM_SETTINGS (object); NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); const GSList *specs, *iter; - GHashTableIter citer; GPtrArray *array; - const char *path; switch (prop_id) { case PROP_UNMANAGED_SPECS: @@ -1894,12 +1892,9 @@ get_property (GObject *object, guint prop_id, g_value_set_boolean (value, !!get_plugin (self, NM_SETTINGS_PLUGIN_CAP_MODIFY_CONNECTIONS)); break; case PROP_CONNECTIONS: - array = g_ptr_array_sized_new (g_hash_table_size (priv->connections) + 1); - g_hash_table_iter_init (&citer, priv->connections); - while (g_hash_table_iter_next (&citer, (gpointer) &path, NULL)) - g_ptr_array_add (array, g_strdup (path)); - g_ptr_array_add (array, NULL); - g_value_take_boxed (value, (char **) g_ptr_array_free (array, FALSE)); + nm_dbus_utils_g_value_set_object_path_from_hash (value, + priv->connections, + TRUE); break; case PROP_STARTUP_COMPLETE: g_value_set_boolean (value, nm_settings_get_startup_complete (self)); From 1b532b5fdc564eae906a47402849087da4edf9f8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 12:09:10 +0200 Subject: [PATCH 15/40] core: minor cleanup of nm_utils_g_value_set_strv() Add some assertion, use an unsigned loop variable (that matches GPtrArray.len type), and add a comment. --- src/nm-core-utils.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 7c36596d0a..2a50f8f816 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -3650,7 +3650,8 @@ nm_utils_setpgid (gpointer unused G_GNUC_UNUSED) /** * nm_utils_g_value_set_strv: * @value: a #GValue, initialized to store a #G_TYPE_STRV - * @strings: a #GPtrArray of strings + * @strings: a #GPtrArray of strings. %NULL values are not + * allowed. * * Converts @strings to a #GStrv and stores it in @value. */ @@ -3658,11 +3659,13 @@ void nm_utils_g_value_set_strv (GValue *value, GPtrArray *strings) { char **strv; - int i; + guint i; strv = g_new (char *, strings->len + 1); - for (i = 0; i < strings->len; i++) + for (i = 0; i < strings->len; i++) { + nm_assert (strings->pdata[i]); strv[i] = g_strdup (strings->pdata[i]); + } strv[i] = NULL; g_value_take_boxed (value, strv); From dcbb5c07e1d4aef1e7653d620eba474b5aba2c24 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 15:03:39 +0200 Subject: [PATCH 16/40] core: drop unused NMConnectionProvider typedef We dopped NMConnectionProvider a while ago. Forgot something. Fixes: 5337003c4cd946860c6bea98164874f8c4aed5e7 --- src/devices/nm-device.c | 6 +++--- src/nm-types.h | 1 - src/tests/config/nm-test-device.c | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 7f22b67d89..86c4b3373a 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -12841,19 +12841,19 @@ cp_connection_added_or_updated (NMDevice *self, NMConnection *connection) } static void -cp_connection_added (NMConnectionProvider *cp, NMConnection *connection, gpointer user_data) +cp_connection_added (NMSettings *settings, NMConnection *connection, gpointer user_data) { cp_connection_added_or_updated (user_data, connection); } static void -cp_connection_updated (NMConnectionProvider *cp, NMConnection *connection, gboolean by_user, gpointer user_data) +cp_connection_updated (NMSettings *settings, NMConnection *connection, gboolean by_user, gpointer user_data) { cp_connection_added_or_updated (user_data, connection); } static void -cp_connection_removed (NMConnectionProvider *cp, NMConnection *connection, gpointer user_data) +cp_connection_removed (NMSettings *settings, NMConnection *connection, gpointer user_data) { NMDevice *self = user_data; diff --git a/src/nm-types.h b/src/nm-types.h index 0d5277e3af..e48a6e2a18 100644 --- a/src/nm-types.h +++ b/src/nm-types.h @@ -38,7 +38,6 @@ typedef struct _NMDBusManager NMDBusManager; typedef struct _NMConfig NMConfig; typedef struct _NMConfigData NMConfigData; typedef struct _NMArpingManager NMArpingManager; -typedef struct _NMConnectionProvider NMConnectionProvider; typedef struct _NMConnectivity NMConnectivity; typedef struct _NMDevice NMDevice; typedef struct _NMDhcp4Config NMDhcp4Config; diff --git a/src/tests/config/nm-test-device.c b/src/tests/config/nm-test-device.c index 3ec866f5e5..4963158340 100644 --- a/src/tests/config/nm-test-device.c +++ b/src/tests/config/nm-test-device.c @@ -57,7 +57,7 @@ nm_test_device_init (NMTestDevice *self) } /* We jump over NMDevice's construct/destruct methods, which require NMPlatform - * and NMConnectionProvider to be initialized. + * and NMSettings to be initialized. */ static void constructed (GObject *object) From ebd53888b6d2c2890f68e0013e15c46cd1239577 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 16:47:05 +0200 Subject: [PATCH 17/40] core: avoid unnecessary action in NMPolicy's _deactivate_if_active() We only need @state, after we verified that the active connection references the right settings connection. Most of the time, that is not the case. --- src/nm-policy.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/nm-policy.c b/src/nm-policy.c index 61efee556b..9f845c1b0e 100644 --- a/src/nm-policy.c +++ b/src/nm-policy.c @@ -2321,10 +2321,9 @@ _deactivate_if_active (NMPolicy *self, NMSettingsConnection *connection) nm_assert (NM_IS_SETTINGS_CONNECTION (connection)); nm_manager_for_each_active_connection (priv->manager, ac, tmp_list) { - NMActiveConnectionState state = nm_active_connection_get_state (ac); if ( nm_active_connection_get_settings_connection (ac) == connection - && (state <= NM_ACTIVE_CONNECTION_STATE_ACTIVATED)) { + && (nm_active_connection_get_state (ac) <= NM_ACTIVE_CONNECTION_STATE_ACTIVATED)) { if (!nm_manager_deactivate_connection (priv->manager, ac, NM_DEVICE_STATE_REASON_CONNECTION_REMOVED, From 1f3b47deea84888813ed482f5b3e75292b0f2726 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 15:01:11 +0200 Subject: [PATCH 18/40] settings: reorder D-Bus events when removing settings-connection It makes more sense to me this way. --- src/settings/nm-settings.c | 41 +++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index dfaf517fc5..e3a9fd4c19 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -848,19 +848,6 @@ connection_flags_changed (NMSettingsConnection *connection, connection); } -static void -_emit_connection_removed (NMSettings *self, - NMSettingsConnection *connection) -{ - nm_dbus_object_emit_signal (NM_DBUS_OBJECT (self), - &interface_info_settings, - &signal_info_connection_removed, - "(o)", - nm_dbus_object_get_path (NM_DBUS_OBJECT (connection))); - - g_signal_emit (self, signals[CONNECTION_REMOVED], 0, connection); -} - static void connection_removed (NMSettingsConnection *connection, gpointer user_data) { @@ -868,13 +855,10 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); const char *cpath = nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)); NMDevice *device; - _nm_unused gs_unref_object NMSettingsConnection *connection_keep_alive = NULL; if (!g_hash_table_lookup (priv->connections, cpath)) g_return_if_reached (); - connection_keep_alive = g_object_ref (connection); - /* When the default wired connection is removed (either deleted or saved to * a new persistent connection by a plugin), write the MAC address of the * wired device to the config file and don't create a new default wired @@ -896,16 +880,27 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) g_signal_handlers_disconnect_by_func (connection, G_CALLBACK (connection_ready_changed), self); g_object_unref (self); - /* Forget about the connection internally */ - g_hash_table_remove (priv->connections, (gpointer) cpath); _clear_connections_cached_list (&priv->connections_cached_list); - _emit_connection_removed (self, connection); + /* we unref @connection below. */ + g_hash_table_steal (priv->connections, (gpointer) cpath); - /* Re-emit for listeners like NMPolicy */ - _notify (self, PROP_CONNECTIONS); - if (nm_dbus_object_is_exported (NM_DBUS_OBJECT (connection))) - nm_dbus_object_unexport (NM_DBUS_OBJECT (connection)); + if (priv->connections_loaded) { + _notify (self, PROP_CONNECTIONS); + + nm_dbus_object_emit_signal (NM_DBUS_OBJECT (self), + &interface_info_settings, + &signal_info_connection_removed, + "(o)", + nm_dbus_object_get_path (NM_DBUS_OBJECT (connection))); + } + + nm_dbus_object_unexport (NM_DBUS_OBJECT (connection)); + + if (priv->connections_loaded) + g_signal_emit (self, signals[CONNECTION_REMOVED], 0, connection); + + g_object_ref (connection); check_startup_complete (self); } From 7595b4f1c7d074e9f6c4dc28e58fcaa627fa02a9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 16:53:12 +0200 Subject: [PATCH 19/40] settings: don't let connection keep NMSettings alive NMSettings already references NMSettingsConnection. Hence, it should not at the same time reference itself. Arguably, during shutdown we do not properly release all NMSettingsConnection. For example, there is no nm_settings_stop(). But that is a bug that needs fixing. No need to keep the NMSettings instance alive here. If this is really necessary, it needs fixing somewhere else. Besides, we know that we leak a lot during shutdown, so this needs more work to do a clean shutdown. --- src/settings/nm-settings.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index e3a9fd4c19..ae3f4e933c 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -878,7 +878,6 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) g_signal_handlers_disconnect_by_func (connection, G_CALLBACK (connection_flags_changed), self); if (!priv->startup_complete) g_signal_handlers_disconnect_by_func (connection, G_CALLBACK (connection_ready_changed), self); - g_object_unref (self); _clear_connections_cached_list (&priv->connections_cached_list); @@ -1000,7 +999,6 @@ claim_connection (NMSettings *self, NMSettingsConnection *connection) /* Evil openconnect migration hack */ openconnect_migrate_hack (NM_CONNECTION (connection)); - g_object_ref (self); /* This one unexports the connection, it needs to run late to give the active * connection a chance to deal with its reference to this settings connection. */ g_signal_connect_after (connection, NM_SETTINGS_CONNECTION_REMOVED, From cd71ec3084d972ab70aa6229fd4752317106d281 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 19:27:24 +0200 Subject: [PATCH 20/40] core: convert NMDBusObject's "path" property to signal "exported-changed" The GObject property "path" exists for the sole reasons so that other components can connect to the "notify::path" signal. However, notifications are blocked by g_object_freeze_notify(), and especially for NMDBusObject we want to make use of that to combine multiple PropertiesChanged events into one. This blocking of the signal is not desired for the case where we wait for "notify::path". Convert that to a signal, which will not be blocked. --- src/devices/nm-device.c | 11 +++---- src/nm-dbus-object.c | 73 ++++++++++++++++++++++++----------------- src/nm-dbus-object.h | 15 ++++++++- src/nm-dbus-utils.c | 14 ++++++++ src/nm-dbus-utils.h | 2 ++ 5 files changed, 77 insertions(+), 38 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 86c4b3373a..651390db60 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -9577,9 +9577,8 @@ nm_device_activate_ip6_state_done (NMDevice *self) /*****************************************************************************/ static void -act_request_set_cb (NMActRequest *act_request, - GParamSpec *pspec, - NMDevice *self) +act_request_exported_changed (NMActRequest *act_request, + NMDevice *self) { _notify (self, PROP_ACTIVE_CONNECTION); } @@ -9610,8 +9609,8 @@ act_request_set (NMDevice *self, NMActRequest *act_request) if (act_request) { priv->act_request_id = g_signal_connect (act_request, - "notify::"NM_DBUS_OBJECT_PATH, - G_CALLBACK (act_request_set_cb), + NM_DBUS_OBJECT_EXPORTED_CHANGED, + G_CALLBACK (act_request_exported_changed), self); switch (nm_active_connection_get_activation_type (NM_ACTIVE_CONNECTION (act_request))) { @@ -15220,7 +15219,7 @@ get_property (GObject *object, guint prop_id, g_variant_new ("(uu)", priv->state, priv->state_reason)); break; case PROP_ACTIVE_CONNECTION: - nm_dbus_utils_g_value_set_object_path (value, priv->act_request_public ? priv->act_request : NULL); + nm_dbus_utils_g_value_set_object_path_still_exported (value, priv->act_request_public ? priv->act_request : NULL); break; case PROP_DEVICE_TYPE: g_value_set_uint (value, priv->type); diff --git a/src/nm-dbus-object.c b/src/nm-dbus-object.c index cd3a23509e..014f30f40c 100644 --- a/src/nm-dbus-object.c +++ b/src/nm-dbus-object.c @@ -37,9 +37,13 @@ nm_dbus_object_set_quitting (void) /*****************************************************************************/ -NM_GOBJECT_PROPERTIES_DEFINE (NMDBusObject, - PROP_PATH, -); +enum { + EXPORTED_CHANGED, + + LAST_SIGNAL, +}; + +static guint signals[LAST_SIGNAL] = { 0 }; G_DEFINE_ABSTRACT_TYPE (NMDBusObject, nm_dbus_object, G_TYPE_OBJECT); @@ -53,6 +57,12 @@ G_DEFINE_ABSTRACT_TYPE (NMDBusObject, nm_dbus_object, G_TYPE_OBJECT); /*****************************************************************************/ +static void +_emit_exported_changed (NMDBusObject *self) +{ + g_signal_emit (self, signals[EXPORTED_CHANGED], 0); +} + static char * _create_export_path (NMDBusObjectClass *klass) { @@ -107,6 +117,8 @@ nm_dbus_object_export (NMDBusObject *self) g_return_val_if_fail (!self->internal.path, self->internal.path); + nm_assert (!self->internal.is_unexporting); + self->internal.path = _create_export_path (NM_DBUS_OBJECT_GET_CLASS (self)); self->internal.export_version_id = ++id_counter; @@ -115,7 +127,7 @@ nm_dbus_object_export (NMDBusObject *self) _nm_dbus_manager_obj_export (self); - _notify (self, PROP_PATH); + _emit_exported_changed (self); return self->internal.path; } @@ -135,12 +147,27 @@ nm_dbus_object_unexport (NMDBusObject *self) _LOGT ("unexport: \"%s\"", self->internal.path); + /* note that we emit the signal *before* actually unexporting the object. + * The reason is, that listeners want to use this signal to know that + * the object goes away, and clear their D-Bus path to this object. + * + * But this must happen before we actually unregister the object, so + * that we first emit a D-Bus signal that other objects no longer + * reference this object, before finally unregistering the object itself. + * + * The inconvenient part is, that at this point nm_dbus_object_get_path() + * still returns the path. So, the callee needs to handle that. Possibly + * by using "nm_dbus_object_get_path_still_exported()". */ + self->internal.is_unexporting = TRUE; + + _emit_exported_changed (self); + _nm_dbus_manager_obj_unexport (self); g_clear_pointer (&self->internal.path, g_free); self->internal.export_version_id = 0; - _notify (self, PROP_PATH); + self->internal.is_unexporting = FALSE; } /*****************************************************************************/ @@ -204,22 +231,6 @@ nm_dbus_object_emit_signal (NMDBusObject *self, /*****************************************************************************/ -static void -get_property (GObject *object, guint prop_id, - GValue *value, GParamSpec *pspec) -{ - NMDBusObject *self = NM_DBUS_OBJECT (object); - - switch (prop_id) { - case PROP_PATH: - g_value_set_string (value, self->internal.path); - break; - default: - G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); - break; - } -} - static void dispatch_properties_changed (GObject *object, guint n_pspecs, @@ -284,10 +295,11 @@ dispose (GObject *object) g_warn_if_reached (); nm_dbus_object_unexport (self); } - } else if (nm_clear_g_free (&self->internal.path)) { + } else if (self->internal.path) { /* FIXME: do a proper, coordinate shutdown, so that no objects stay * alive nor exported. */ - _notify (self, PROP_PATH); + _emit_exported_changed (self); + nm_clear_g_free (&self->internal.path); } G_OBJECT_CLASS (nm_dbus_object_parent_class)->dispose (object); @@ -302,14 +314,13 @@ nm_dbus_object_class_init (NMDBusObjectClass *klass) object_class->constructed = constructed; object_class->dispose = dispose; - object_class->get_property = get_property; object_class->dispatch_properties_changed = dispatch_properties_changed; - obj_properties[PROP_PATH] = - g_param_spec_string (NM_DBUS_OBJECT_PATH, "", "", - NULL, - G_PARAM_READABLE | - G_PARAM_STATIC_STRINGS); - - g_object_class_install_properties (object_class, _PROPERTY_ENUMS_LAST, obj_properties); + signals[EXPORTED_CHANGED] = + g_signal_new (NM_DBUS_OBJECT_EXPORTED_CHANGED, + G_OBJECT_CLASS_TYPE (object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, + g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0); } diff --git a/src/nm-dbus-object.h b/src/nm-dbus-object.h index f6629289a5..947ea6c7dd 100644 --- a/src/nm-dbus-object.h +++ b/src/nm-dbus-object.h @@ -82,7 +82,7 @@ extern const NMDBusInterfaceInfoExtended nm_interface_info_device_statistics; #define NM_IS_DBUS_OBJECT_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), NM_TYPE_DBUS_OBJECT)) #define NM_DBUS_OBJECT_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), NM_TYPE_DBUS_OBJECT, NMDBusObjectClass)) -#define NM_DBUS_OBJECT_PATH "path" +#define NM_DBUS_OBJECT_EXPORTED_CHANGED "exported-changed" /* NMDBusObject and NMDBusManager cooperate strongly. Hence, there is an * internal data structure attached to the NMDBusObject accessible to both of them. */ @@ -99,6 +99,7 @@ struct _NMDBusObjectInternal { * unexported, or even re-exported afterwards. If that happens, we want * to fail the request. For that, we keep track of a version id. */ guint64 export_version_id; + bool is_unexporting:1; }; struct _NMDBusObject { @@ -169,6 +170,18 @@ nm_dbus_object_is_exported (NMDBusObject *self) return !!nm_dbus_object_get_path (self); } +static inline const char * +nm_dbus_object_get_path_still_exported (NMDBusObject *self) +{ + g_return_val_if_fail (NM_IS_DBUS_OBJECT (self), NULL); + + /* like nm_dbus_object_get_path(), however, while unexporting + * (exported-changed signal), returns %NULL instead of the path. */ + return self->internal.is_unexporting + ? NULL + : self->internal.path; +} + const char *nm_dbus_object_export (NMDBusObject *self); void nm_dbus_object_unexport (NMDBusObject *self); diff --git a/src/nm-dbus-utils.c b/src/nm-dbus-utils.c index 7f7fb27873..b79cc34371 100644 --- a/src/nm-dbus-utils.c +++ b/src/nm-dbus-utils.c @@ -120,6 +120,20 @@ nm_dbus_utils_g_value_set_object_path (GValue *value, gpointer object) g_value_set_string (value, "/"); } +void +nm_dbus_utils_g_value_set_object_path_still_exported (GValue *value, gpointer object) +{ + const char *path; + + g_return_if_fail (!object || NM_IS_DBUS_OBJECT (object)); + + if ( object + && (path = nm_dbus_object_get_path_still_exported (object))) + g_value_set_string (value, path); + else + g_value_set_string (value, "/"); +} + void nm_dbus_utils_g_value_set_object_path_from_hash (GValue *value, GHashTable *hash /* has keys of NMDBusObject type. */, diff --git a/src/nm-dbus-utils.h b/src/nm-dbus-utils.h index 62945cf14e..3107a29c82 100644 --- a/src/nm-dbus-utils.h +++ b/src/nm-dbus-utils.h @@ -173,6 +173,8 @@ const char **nm_dbus_utils_get_paths_for_clist (const struct CList *lst_head, void nm_dbus_utils_g_value_set_object_path (GValue *value, gpointer object); +void nm_dbus_utils_g_value_set_object_path_still_exported (GValue *value, gpointer object); + void nm_dbus_utils_g_value_set_object_path_from_hash (GValue *value, GHashTable *hash, gboolean expect_all_exported); From 0dd3e6099cfcf4716b19336f2328fc7eff75cde5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 19:16:03 +0200 Subject: [PATCH 21/40] core: don't unexport active-connection when settings connection disappears When a settings-connection gets deleted, we need to bring down the NMActiveConnection that contains it. However, we shouldn't just unexport the active connection from D-Bus. Instead, clear the settings path. We need to drop the path, because the connection is going away. It's a bit ugly, that an active-connection might reference no settings-connection. However, this only happens during shut-down. The alternative, would be to keep the settings-connection object in a zombie state, exported on D-Bus. However, that seems even more confusing to me. --- src/nm-active-connection.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index e78caaf28f..dc579487d8 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -184,18 +184,10 @@ _settings_connection_updated (NMSettingsConnection *connection, } static void -_settings_connection_removed (NMSettingsConnection *connection, - gpointer user_data) +_settings_connection_exported_changed (NMSettingsConnection *settings_connection, + NMActiveConnection *self) { - NMActiveConnection *self = user_data; - - /* Our settings connection is about to drop off. The next active connection - * cleanup is going to tear us down (at least until we grow the capability to - * re-link; in that case we'd just clean the references to the old connection here). - * Let's remove ourselves from the bus so that we're not exposed with a dangling - * reference to the setting connection once it's gone. */ - if (nm_dbus_object_is_exported (NM_DBUS_OBJECT (self))) - nm_dbus_object_unexport (NM_DBUS_OBJECT (self)); + _notify (self, PROP_CONNECTION); } static void @@ -207,14 +199,14 @@ _set_settings_connection (NMActiveConnection *self, NMSettingsConnection *connec return; if (priv->settings_connection) { g_signal_handlers_disconnect_by_func (priv->settings_connection, _settings_connection_updated, self); - g_signal_handlers_disconnect_by_func (priv->settings_connection, _settings_connection_removed, self); + g_signal_handlers_disconnect_by_func (priv->settings_connection, _settings_connection_exported_changed, self); g_signal_handlers_disconnect_by_func (priv->settings_connection, _settings_connection_notify_flags, self); g_clear_object (&priv->settings_connection); } if (connection) { priv->settings_connection = g_object_ref (connection); g_signal_connect (connection, NM_SETTINGS_CONNECTION_UPDATED_INTERNAL, (GCallback) _settings_connection_updated, self); - g_signal_connect (connection, NM_SETTINGS_CONNECTION_REMOVED, (GCallback) _settings_connection_removed, self); + g_signal_connect (connection, NM_DBUS_OBJECT_EXPORTED_CHANGED, G_CALLBACK (_settings_connection_exported_changed), self); if (nm_active_connection_get_activation_type (self) == NM_ACTIVATION_TYPE_EXTERNAL) g_signal_connect (connection, "notify::"NM_SETTINGS_CONNECTION_FLAGS, (GCallback) _settings_connection_notify_flags, self); } @@ -1186,7 +1178,7 @@ get_property (GObject *object, guint prop_id, * is set, to get an assertion failure if somebody tries to access the * getters at the wrong time. */ case PROP_CONNECTION: - g_value_set_string (value, nm_dbus_object_get_path (NM_DBUS_OBJECT (priv->settings_connection))); + nm_dbus_utils_g_value_set_object_path_still_exported (value, priv->settings_connection); break; case PROP_ID: g_value_set_string (value, nm_connection_get_id (NM_CONNECTION (priv->settings_connection))); From 4127f1234fc4593cb80ad01435ac2b1743c8d5e2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 14:48:42 +0200 Subject: [PATCH 22/40] settings: track connections via CList --- src/settings/nm-settings-connection.c | 4 + src/settings/nm-settings-connection.h | 1 + src/settings/nm-settings.c | 242 ++++++++++++-------------- src/settings/nm-settings.h | 8 - 4 files changed, 121 insertions(+), 134 deletions(-) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 75c4f388ce..88f9c18085 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -2965,6 +2965,8 @@ nm_settings_connection_init (NMSettingsConnection *self) priv = G_TYPE_INSTANCE_GET_PRIVATE (self, NM_TYPE_SETTINGS_CONNECTION, NMSettingsConnectionPrivate); self->_priv = priv; + c_list_init (&self->_connections_lst); + priv->ready = TRUE; c_list_init (&priv->call_ids_lst_head); @@ -3002,6 +3004,8 @@ dispose (GObject *object) _LOGD ("disposing"); + nm_assert (c_list_is_empty (&self->_connections_lst)); + /* Cancel in-progress secrets requests */ if (priv->agent_mgr) { c_list_for_each_entry_safe (call_id, call_id_safe, &priv->call_ids_lst_head, call_ids_lst) diff --git a/src/settings/nm-settings-connection.h b/src/settings/nm-settings-connection.h index f5b3179312..555a1d1285 100644 --- a/src/settings/nm-settings-connection.h +++ b/src/settings/nm-settings-connection.h @@ -104,6 +104,7 @@ struct _NMSettingsConnectionPrivate; struct _NMSettingsConnection { NMDBusObject parent; struct _NMSettingsConnectionPrivate *_priv; + CList _connections_lst; }; struct _NMSettingsConnectionClass { diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index ae3f4e933c..67e035f038 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -62,6 +62,7 @@ #include "nm-utils.h" #include "nm-core-internal.h" +#include "nm-utils/nm-c-list.h" #include "nm-dbus-object.h" #include "devices/nm-device-ethernet.h" #include "nm-settings-connection.h" @@ -119,17 +120,21 @@ typedef struct { GSList *auths; GSList *plugins; - gboolean connections_loaded; - GHashTable *connections; + + CList connections_lst_head; + NMSettingsConnection **connections_cached_list; GSList *unmanaged_specs; GSList *unrecognized_specs; - gboolean started; - gboolean startup_complete; - NMHostnameManager *hostname_manager; + guint connections_len; + + bool started:1; + bool startup_complete:1; + bool connections_loaded:1; + } NMSettingsPrivate; struct _NMSettings { @@ -177,21 +182,18 @@ static void check_startup_complete (NMSettings *self) { NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); - GHashTableIter iter; NMSettingsConnection *conn; if (priv->startup_complete) return; - g_hash_table_iter_init (&iter, priv->connections); - while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &conn)) { + c_list_for_each_entry (conn, &priv->connections_lst_head, _connections_lst) { if (!nm_settings_connection_get_ready (conn)) return; } /* the connection_ready_changed signal handler is no longer needed. */ - g_hash_table_iter_init (&iter, priv->connections); - while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &conn)) + c_list_for_each_entry (conn, &priv->connections_lst_head, _connections_lst) g_signal_handlers_disconnect_by_func (conn, G_CALLBACK (connection_ready_changed), self); priv->startup_complete = TRUE; @@ -254,48 +256,25 @@ load_connections (NMSettings *self) unrecognized_specs_changed (NULL, self); } -void -nm_settings_for_each_connection (NMSettings *self, - NMSettingsForEachFunc for_each_func, - gpointer user_data) -{ - NMSettingsPrivate *priv; - GHashTableIter iter; - gpointer data; - - g_return_if_fail (NM_IS_SETTINGS (self)); - g_return_if_fail (for_each_func != NULL); - - priv = NM_SETTINGS_GET_PRIVATE (self); - - g_hash_table_iter_init (&iter, priv->connections); - while (g_hash_table_iter_next (&iter, NULL, &data)) - for_each_func (self, NM_SETTINGS_CONNECTION (data), user_data); -} - static void impl_settings_list_connections (NMDBusObject *obj, const NMDBusInterfaceInfoExtended *interface_info, const NMDBusMethodInfoExtended *method_info, - GDBusConnection *connection, + GDBusConnection *dbus_connection, const char *sender, GDBusMethodInvocation *invocation, GVariant *parameters) { NMSettings *self = NM_SETTINGS (obj); NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); - gs_unref_ptrarray GPtrArray *connections = NULL; - GHashTableIter iter; - gpointer key; - - connections = g_ptr_array_sized_new (g_hash_table_size (priv->connections) + 1); - g_hash_table_iter_init (&iter, priv->connections); - while (g_hash_table_iter_next (&iter, &key, NULL)) - g_ptr_array_add (connections, key); - g_ptr_array_add (connections, NULL); + gs_free const char **strv = NULL; + strv = nm_dbus_utils_get_paths_for_clist (&priv->connections_lst_head, + priv->connections_len, + G_STRUCT_OFFSET (NMSettingsConnection, _connections_lst), + TRUE); g_dbus_method_invocation_return_value (invocation, - g_variant_new ("(^ao)", connections->pdata)); + g_variant_new ("(^ao)", strv)); } NMSettingsConnection * @@ -303,16 +282,14 @@ nm_settings_get_connection_by_uuid (NMSettings *self, const char *uuid) { NMSettingsPrivate *priv; NMSettingsConnection *candidate; - GHashTableIter iter; g_return_val_if_fail (NM_IS_SETTINGS (self), NULL); g_return_val_if_fail (uuid != NULL, NULL); priv = NM_SETTINGS_GET_PRIVATE (self); - g_hash_table_iter_init (&iter, priv->connections); - while (g_hash_table_iter_next (&iter, NULL, (gpointer) &candidate)) { - if (g_strcmp0 (uuid, nm_settings_connection_get_uuid (candidate)) == 0) + c_list_for_each_entry (candidate, &priv->connections_lst_head, _connections_lst) { + if (nm_streq (uuid, nm_settings_connection_get_uuid (candidate))) return candidate; } @@ -373,19 +350,23 @@ error: } static void -_clear_connections_cached_list (NMSettingsConnection ***p_connections_cached_list) +_clear_connections_cached_list (NMSettingsPrivate *priv) { + if (!priv->connections_cached_list) + return; + + nm_assert (priv->connections_len == NM_PTRARRAY_LEN (priv->connections_cached_list)); + #if NM_MORE_ASSERTS /* set the pointer to a bogus value. This makes it more apparent * if somebody has a reference to the cached list and still uses * it. That is a bug, this code just tries to make it blow up * more eagerly. */ - if (*p_connections_cached_list) { - NMSettingsConnection **p = *p_connections_cached_list; - memset (p, 0xdeaddead, sizeof (NMSettingsConnection *) * (NM_PTRARRAY_LEN (p) + 1)); - } + memset (priv->connections_cached_list, + 0xdeaddead, + sizeof (NMSettingsConnection *) * (priv->connections_len + 1)); #endif - g_clear_pointer (p_connections_cached_list, g_free); + nm_clear_g_free (&priv->connections_cached_list); } /** @@ -403,37 +384,33 @@ _clear_connections_cached_list (NMSettingsConnection ***p_connections_cached_lis NMSettingsConnection *const* nm_settings_get_connections (NMSettings *self, guint *out_len) { - GHashTableIter iter; NMSettingsPrivate *priv; - guint l, i; NMSettingsConnection **v; NMSettingsConnection *con; + guint i; g_return_val_if_fail (NM_IS_SETTINGS (self), NULL); priv = NM_SETTINGS_GET_PRIVATE (self); - if (G_LIKELY (priv->connections_cached_list)) { - NM_SET_OUT (out_len, g_hash_table_size (priv->connections)); - return priv->connections_cached_list; + nm_assert (priv->connections_len == c_list_length (&priv->connections_lst_head)); + + if (G_UNLIKELY (!priv->connections_cached_list)) { + v = g_new (NMSettingsConnection *, priv->connections_len + 1); + + i = 0; + c_list_for_each_entry (con, &priv->connections_lst_head, _connections_lst) { + nm_assert (i < priv->connections_len); + v[i++] = con; + } + nm_assert (i == priv->connections_len); + v[i] = NULL; + + priv->connections_cached_list = v; } - l = g_hash_table_size (priv->connections); - - v = g_new (NMSettingsConnection *, (gsize) l + 1); - - i = 0; - g_hash_table_iter_init (&iter, priv->connections); - while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &con)) { - nm_assert (i < l); - v[i++] = con; - } - nm_assert (i == l); - v[i] = NULL; - - NM_SET_OUT (out_len, l); - priv->connections_cached_list = v; - return v; + NM_SET_OUT (out_len, priv->connections_len); + return priv->connections_cached_list; } /** @@ -500,28 +477,41 @@ NMSettingsConnection * nm_settings_get_connection_by_path (NMSettings *self, const char *path) { NMSettingsPrivate *priv; + NMSettingsConnection *connection; g_return_val_if_fail (NM_IS_SETTINGS (self), NULL); - g_return_val_if_fail (path != NULL, NULL); + g_return_val_if_fail (path, NULL); priv = NM_SETTINGS_GET_PRIVATE (self); - return (NMSettingsConnection *) g_hash_table_lookup (priv->connections, path); + connection = (NMSettingsConnection *) nm_dbus_manager_lookup_object (nm_dbus_object_get_manager (NM_DBUS_OBJECT (self)), + path); + if ( !connection + || !NM_IS_SETTINGS_CONNECTION (connection)) + return NULL; + + nm_assert (c_list_contains (&priv->connections_lst_head, &connection->_connections_lst)); + return connection; } gboolean nm_settings_has_connection (NMSettings *self, NMSettingsConnection *connection) { - NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); - GHashTableIter iter; - gpointer data; + NMSettingsConnection *candidate = NULL; + const char *path; - g_hash_table_iter_init (&iter, priv->connections); - while (g_hash_table_iter_next (&iter, NULL, &data)) - if (data == connection) - return TRUE; + g_return_val_if_fail (NM_IS_SETTINGS (self), FALSE); + g_return_val_if_fail (NM_IS_SETTINGS_CONNECTION (connection), FALSE); - return FALSE; + path = nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)); + if (path) + candidate = nm_settings_get_connection_by_path (self, path); + + nm_assert (!candidate || candidate == connection); + nm_assert (!!candidate == nm_c_list_contains_entry (&NM_SETTINGS_GET_PRIVATE (self)->connections_lst_head, + connection, + _connections_lst)); + return !!candidate; } const GSList * @@ -853,11 +843,11 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) { NMSettings *self = NM_SETTINGS (user_data); NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); - const char *cpath = nm_dbus_object_get_path (NM_DBUS_OBJECT (connection)); NMDevice *device; - if (!g_hash_table_lookup (priv->connections, cpath)) - g_return_if_reached (); + g_return_if_fail (NM_IS_SETTINGS_CONNECTION (connection)); + g_return_if_fail (!c_list_is_empty (&connection->_connections_lst)); + nm_assert (c_list_contains (&priv->connections_lst_head, &connection->_connections_lst)); /* When the default wired connection is removed (either deleted or saved to * a new persistent connection by a plugin), write the MAC address of the @@ -879,10 +869,10 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) if (!priv->startup_complete) g_signal_handlers_disconnect_by_func (connection, G_CALLBACK (connection_ready_changed), self); - _clear_connections_cached_list (&priv->connections_cached_list); - - /* we unref @connection below. */ - g_hash_table_steal (priv->connections, (gpointer) cpath); + /* Forget about the connection internally */ + _clear_connections_cached_list (priv); + priv->connections_len--; + c_list_unlink (&connection->_connections_lst); if (priv->connections_loaded) { _notify (self, PROP_CONNECTIONS); @@ -949,19 +939,16 @@ claim_connection (NMSettings *self, NMSettingsConnection *connection) { NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); GError *error = NULL; - GHashTableIter iter; - gpointer data; const char *path; NMSettingsConnection *existing; g_return_if_fail (NM_IS_SETTINGS_CONNECTION (connection)); g_return_if_fail (!nm_dbus_object_is_exported (NM_DBUS_OBJECT (connection))); - g_hash_table_iter_init (&iter, priv->connections); - while (g_hash_table_iter_next (&iter, NULL, &data)) { - /* prevent duplicates */ - if (data == connection) - return; + /* prevent duplicates */ + if (!c_list_is_empty (&connection->_connections_lst)) { + nm_assert (c_list_contains (&priv->connections_lst_head, &connection->_connections_lst)); + return; } if (!nm_connection_normalize (NM_CONNECTION (connection), NULL, NULL, &error)) { @@ -1014,12 +1001,12 @@ claim_connection (NMSettings *self, NMSettingsConnection *connection) self); } - path = nm_dbus_object_export (NM_DBUS_OBJECT (connection)); + _clear_connections_cached_list (priv); - g_hash_table_insert (priv->connections, - (gpointer) path, - g_object_ref (connection)); - _clear_connections_cached_list (&priv->connections_cached_list); + priv->connections_len++; + c_list_link_tail (&priv->connections_lst_head, &connection->_connections_lst); + + path = nm_dbus_object_export (NM_DBUS_OBJECT (connection)); nm_utils_log_connection_diff (NM_CONNECTION (connection), NULL, LOGL_DEBUG, LOGD_CORE, "new connection", "++ ", path); @@ -1083,14 +1070,14 @@ nm_settings_add_connection (NMSettings *self, NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); GSList *iter; NMSettingsConnection *added = NULL; - GHashTableIter citer; - NMConnection *candidate = NULL; + NMSettingsConnection *candidate = NULL; + const char *uuid; + + uuid = nm_connection_get_uuid (connection); /* Make sure a connection with this UUID doesn't already exist */ - g_hash_table_iter_init (&citer, priv->connections); - while (g_hash_table_iter_next (&citer, NULL, (gpointer *) &candidate)) { - if (g_strcmp0 (nm_connection_get_uuid (connection), - nm_connection_get_uuid (candidate)) == 0) { + c_list_for_each_entry (candidate, &priv->connections_lst_head, _connections_lst) { + if (nm_streq0 (uuid, nm_connection_get_uuid (NM_CONNECTION (candidate)))) { g_set_error_literal (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_UUID_EXISTS, @@ -1624,27 +1611,24 @@ static gboolean have_connection_for_device (NMSettings *self, NMDevice *device) { NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); - GHashTableIter iter; - gpointer data; NMSettingConnection *s_con; NMSettingWired *s_wired; const char *setting_hwaddr; const char *perm_hw_addr; + NMSettingsConnection *connection; g_return_val_if_fail (NM_IS_SETTINGS (self), FALSE); perm_hw_addr = nm_device_get_permanent_hw_address (device); /* Find a wired connection locked to the given MAC address, if any */ - g_hash_table_iter_init (&iter, priv->connections); - while (g_hash_table_iter_next (&iter, NULL, &data)) { - NMConnection *connection = NM_CONNECTION (data); + c_list_for_each_entry (connection, &priv->connections_lst_head, _connections_lst) { const char *ctype, *iface; - if (!nm_device_check_connection_compatible (device, connection)) + if (!nm_device_check_connection_compatible (device, NM_CONNECTION (connection))) continue; - s_con = nm_connection_get_setting_connection (connection); + s_con = nm_connection_get_setting_connection (NM_CONNECTION (connection)); iface = nm_setting_connection_get_interface_name (s_con); if (iface && strcmp (iface, nm_device_get_iface (device)) != 0) @@ -1655,7 +1639,7 @@ have_connection_for_device (NMSettings *self, NMDevice *device) && strcmp (ctype, NM_SETTING_PPPOE_SETTING_NAME)) continue; - s_wired = nm_connection_get_setting_wired (connection); + s_wired = nm_connection_get_setting_wired (NM_CONNECTION (connection)); if (!s_wired && !strcmp (ctype, NM_SETTING_PPPOE_SETTING_NAME)) { /* No wired setting; therefore the PPPoE connection applies to any device */ @@ -1864,16 +1848,19 @@ get_property (GObject *object, guint prop_id, NMSettings *self = NM_SETTINGS (object); NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); const GSList *specs, *iter; - GPtrArray *array; + guint i; + char **strvs; + const char **strv; switch (prop_id) { case PROP_UNMANAGED_SPECS: - array = g_ptr_array_new (); specs = nm_settings_get_unmanaged_specs (self); - for (iter = specs; iter; iter = g_slist_next (iter)) - g_ptr_array_add (array, g_strdup (iter->data)); - g_ptr_array_add (array, NULL); - g_value_take_boxed (value, (char **) g_ptr_array_free (array, FALSE)); + strvs = g_new (char *, g_slist_length ((GSList *) specs) + 1); + i = 0; + for (iter = specs; iter; iter = iter->next) + strvs[i++] = g_strdup (iter->data); + strvs[i] = NULL; + g_value_take_boxed (value, strvs); break; case PROP_HOSTNAME: g_value_set_string (value, @@ -1885,9 +1872,11 @@ get_property (GObject *object, guint prop_id, g_value_set_boolean (value, !!get_plugin (self, NM_SETTINGS_PLUGIN_CAP_MODIFY_CONNECTIONS)); break; case PROP_CONNECTIONS: - nm_dbus_utils_g_value_set_object_path_from_hash (value, - priv->connections, - TRUE); + strv = nm_dbus_utils_get_paths_for_clist (&priv->connections_lst_head, + priv->connections_len, + G_STRUCT_OFFSET (NMSettingsConnection, _connections_lst), + TRUE); + g_value_take_boxed (value, nm_utils_strv_make_deep_copied (strv)); break; case PROP_STARTUP_COMPLETE: g_value_set_boolean (value, nm_settings_get_startup_complete (self)); @@ -1905,7 +1894,7 @@ nm_settings_init (NMSettings *self) { NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); - priv->connections = g_hash_table_new_full (nm_str_hash, g_str_equal, NULL, g_object_unref); + c_list_init (&priv->connections_lst_head); priv->agent_mgr = g_object_ref (nm_agent_manager_get ()); priv->config = g_object_ref (nm_config_get ()); @@ -1944,8 +1933,9 @@ finalize (GObject *object) NMSettings *self = NM_SETTINGS (object); NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); - g_hash_table_destroy (priv->connections); - _clear_connections_cached_list (&priv->connections_cached_list); + _clear_connections_cached_list (priv); + + nm_assert (c_list_is_empty (&priv->connections_lst_head)); g_slist_free_full (priv->unmanaged_specs, g_free); g_slist_free_full (priv->unrecognized_specs, g_free); diff --git a/src/settings/nm-settings.h b/src/settings/nm-settings.h index 2a5a5053c5..7d56f7b43f 100644 --- a/src/settings/nm-settings.h +++ b/src/settings/nm-settings.h @@ -70,14 +70,6 @@ NMSettings *nm_settings_get (void); NMSettings *nm_settings_new (void); gboolean nm_settings_start (NMSettings *self, GError **error); -typedef void (*NMSettingsForEachFunc) (NMSettings *settings, - NMSettingsConnection *connection, - gpointer user_data); - -void nm_settings_for_each_connection (NMSettings *settings, - NMSettingsForEachFunc for_each_func, - gpointer user_data); - typedef void (*NMSettingsAddCallback) (NMSettings *settings, NMSettingsConnection *connection, GError *error, From 86229a669bc3c51faf19f15c6a382c20f849b78e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Apr 2018 16:37:20 +0200 Subject: [PATCH 23/40] core: add NMDBusTrackObjPath helper When one D-Bus object exposes (the path of) another D-Bus object, we want that the path property gets cleared before the other object gets unexported(). That essentially requires to register to the "exported-changed" signal. Add a helper struct NMDBusTrackObjPath to help with this. --- src/nm-dbus-utils.c | 99 +++++++++++++++++++++++++++++++++++++++++++++ src/nm-dbus-utils.h | 30 ++++++++++++++ 2 files changed, 129 insertions(+) diff --git a/src/nm-dbus-utils.c b/src/nm-dbus-utils.c index b79cc34371..5a57206c76 100644 --- a/src/nm-dbus-utils.c +++ b/src/nm-dbus-utils.c @@ -207,3 +207,102 @@ nm_dbus_utils_get_paths_for_clist (const CList *lst_head, } /*****************************************************************************/ + +void +nm_dbus_track_obj_path_init (NMDBusTrackObjPath *track, + GObject *target, + const GParamSpec *pspec) +{ + nm_assert (track); + nm_assert (G_IS_OBJECT (target)); + nm_assert (G_IS_PARAM_SPEC (pspec)); + + track->_obj = NULL; + track->_notify_target = target; + track->_notify_pspec = pspec; + track->_notify_signal_id = 0; + track->_visible = FALSE; +} + +void +nm_dbus_track_obj_path_deinit (NMDBusTrackObjPath *track) +{ + /* we allow deinit() to be called multiple times (e.g. from + * dispose(), which must be re-entrant). */ + nm_assert (track); + nm_assert (!track->_notify_target || G_IS_OBJECT (track->_notify_target)); + + nm_clear_g_signal_handler (track->obj, &track->_notify_signal_id); + track->_notify_target = NULL; + track->_notify_pspec = NULL; + track->_visible = FALSE; + nm_clear_g_object (&track->_obj); +} + +void +nm_dbus_track_obj_path_notify (const NMDBusTrackObjPath *track) +{ + nm_assert (track); + nm_assert (G_IS_OBJECT (track->_notify_target)); + nm_assert (G_IS_PARAM_SPEC (track->_notify_pspec)); + + g_object_notify_by_pspec (track->_notify_target, + (GParamSpec *) track->_notify_pspec); +} + +const char * +nm_dbus_track_obj_path_get (const NMDBusTrackObjPath *track) +{ + nm_assert (track); + nm_assert (G_IS_OBJECT (track->_notify_target)); + + return track->obj && track->visible + ? nm_dbus_object_get_path_still_exported (track->obj) + : NULL; +} + +static void +_track_obj_exported_changed (NMDBusObject *obj, + NMDBusTrackObjPath *track) +{ + nm_dbus_track_obj_path_notify (track); +} + +void +nm_dbus_track_obj_path_set (NMDBusTrackObjPath *track, + gpointer obj, + gboolean visible) +{ + gs_unref_object NMDBusObject *old_obj = NULL; + const char *old_path; + + nm_assert (track); + nm_assert (G_IS_OBJECT (track->_notify_target)); + + g_return_if_fail (!obj || NM_IS_DBUS_OBJECT (obj)); + + if ( track->obj == obj + && track->visible == !!visible) + return; + + old_path = nm_dbus_track_obj_path_get (track); + + track->_visible = visible; + + if (track->obj != obj) { + nm_clear_g_signal_handler (track->obj, &track->_notify_signal_id); + + old_obj = track->obj; + track->_obj = nm_g_object_ref (obj); + + if (obj) { + track->_notify_signal_id = g_signal_connect (obj, + NM_DBUS_OBJECT_EXPORTED_CHANGED, + G_CALLBACK (_track_obj_exported_changed), + track); + } + } + + if (!nm_streq0 (old_path, nm_dbus_track_obj_path_get (track))) + nm_dbus_track_obj_path_notify (track); +} diff --git a/src/nm-dbus-utils.h b/src/nm-dbus-utils.h index 3107a29c82..e7e930e938 100644 --- a/src/nm-dbus-utils.h +++ b/src/nm-dbus-utils.h @@ -179,4 +179,34 @@ void nm_dbus_utils_g_value_set_object_path_from_hash (GValue *value, GHashTable *hash, gboolean expect_all_exported); +/*****************************************************************************/ + +typedef struct { + union { + gpointer const obj; + gpointer _obj; + }; + GObject *_notify_target; + const GParamSpec *_notify_pspec; + gulong _notify_signal_id; + union { + const bool visible; + bool _visible; + }; +} NMDBusTrackObjPath; + +void nm_dbus_track_obj_path_init (NMDBusTrackObjPath *track, + GObject *target, + const GParamSpec *pspec); + +void nm_dbus_track_obj_path_deinit (NMDBusTrackObjPath *track); + +void nm_dbus_track_obj_path_notify (const NMDBusTrackObjPath *track); + +const char *nm_dbus_track_obj_path_get (const NMDBusTrackObjPath *track); + +void nm_dbus_track_obj_path_set (NMDBusTrackObjPath *track, + gpointer obj, + gboolean visible); + #endif /* __NM_DBUS_UTILS_H__ */ From 1acb3622aae507eb8df1167f48c80ef333efba2a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 14:48:42 +0200 Subject: [PATCH 24/40] core: use NMDBusTrackObjPath for NM_ACTIVE_CONNECTION_CONNECTION path --- src/nm-active-connection.c | 86 +++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 39 deletions(-) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index dc579487d8..e182e65df9 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -33,7 +33,7 @@ #include "nm-core-internal.h" typedef struct _NMActiveConnectionPrivate { - NMSettingsConnection *settings_connection; + NMDBusTrackObjPath settings_connection; NMConnection *applied_connection; char *specific_object; NMDevice *device; @@ -183,33 +183,25 @@ _settings_connection_updated (NMSettingsConnection *connection, * settings-connection. */ } -static void -_settings_connection_exported_changed (NMSettingsConnection *settings_connection, - NMActiveConnection *self) -{ - _notify (self, PROP_CONNECTION); -} - static void _set_settings_connection (NMActiveConnection *self, NMSettingsConnection *connection) { NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); - if (priv->settings_connection == connection) + if (priv->settings_connection.obj == connection) return; - if (priv->settings_connection) { - g_signal_handlers_disconnect_by_func (priv->settings_connection, _settings_connection_updated, self); - g_signal_handlers_disconnect_by_func (priv->settings_connection, _settings_connection_exported_changed, self); - g_signal_handlers_disconnect_by_func (priv->settings_connection, _settings_connection_notify_flags, self); - g_clear_object (&priv->settings_connection); + + if (priv->settings_connection.obj) { + g_signal_handlers_disconnect_by_func (priv->settings_connection.obj, _settings_connection_updated, self); + g_signal_handlers_disconnect_by_func (priv->settings_connection.obj, _settings_connection_notify_flags, self); } if (connection) { - priv->settings_connection = g_object_ref (connection); g_signal_connect (connection, NM_SETTINGS_CONNECTION_UPDATED_INTERNAL, (GCallback) _settings_connection_updated, self); - g_signal_connect (connection, NM_DBUS_OBJECT_EXPORTED_CHANGED, G_CALLBACK (_settings_connection_exported_changed), self); if (nm_active_connection_get_activation_type (self) == NM_ACTIVATION_TYPE_EXTERNAL) g_signal_connect (connection, "notify::"NM_SETTINGS_CONNECTION_FLAGS, (GCallback) _settings_connection_notify_flags, self); } + + nm_dbus_track_obj_path_set (&priv->settings_connection, connection, TRUE); } NMActiveConnectionState @@ -267,7 +259,7 @@ nm_active_connection_set_state (NMActiveConnection *self, if ( new_state == NM_ACTIVE_CONNECTION_STATE_ACTIVATED || old_state == NM_ACTIVE_CONNECTION_STATE_ACTIVATED) { - nm_settings_connection_update_timestamp (priv->settings_connection, + nm_settings_connection_update_timestamp (priv->settings_connection.obj, (guint64) time (NULL), TRUE); } @@ -363,7 +355,7 @@ nm_active_connection_get_settings_connection_id (NMActiveConnection *self) g_return_val_if_fail (NM_IS_ACTIVE_CONNECTION (self), NULL); - con = NM_ACTIVE_CONNECTION_GET_PRIVATE (self)->settings_connection; + con = NM_ACTIVE_CONNECTION_GET_PRIVATE (self)->settings_connection.obj; return con ? nm_connection_get_id (NM_CONNECTION (con)) : NULL; @@ -374,7 +366,7 @@ _nm_active_connection_get_settings_connection (NMActiveConnection *self) { g_return_val_if_fail (NM_IS_ACTIVE_CONNECTION (self), NULL); - return NM_ACTIVE_CONNECTION_GET_PRIVATE (self)->settings_connection; + return NM_ACTIVE_CONNECTION_GET_PRIVATE (self)->settings_connection.obj; } NMSettingsConnection * @@ -448,7 +440,7 @@ nm_active_connection_set_settings_connection (NMActiveConnection *self, priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); g_return_if_fail (NM_IS_SETTINGS_CONNECTION (connection)); - g_return_if_fail (!priv->settings_connection); + g_return_if_fail (!priv->settings_connection.obj); g_return_if_fail (!priv->applied_connection); /* Can't change connection after the ActiveConnection is exported over D-Bus. @@ -463,7 +455,7 @@ nm_active_connection_set_settings_connection (NMActiveConnection *self, _set_settings_connection (self, connection); _set_applied_connection_take (self, - nm_simple_connection_new_clone (NM_CONNECTION (priv->settings_connection))); + nm_simple_connection_new_clone (NM_CONNECTION (priv->settings_connection.obj))); } gboolean @@ -475,9 +467,9 @@ nm_active_connection_has_unmodified_applied_connection (NMActiveConnection *self priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); - g_return_val_if_fail (priv->settings_connection, FALSE); + g_return_val_if_fail (priv->settings_connection.obj, FALSE); - return nm_settings_connection_has_unmodified_applied_connection (priv->settings_connection, + return nm_settings_connection_has_unmodified_applied_connection (priv->settings_connection.obj, priv->applied_connection, compare_flags); } @@ -493,10 +485,10 @@ nm_active_connection_clear_secrets (NMActiveConnection *self) priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); - if (nm_settings_connection_has_unmodified_applied_connection (priv->settings_connection, + if (nm_settings_connection_has_unmodified_applied_connection (priv->settings_connection.obj, priv->applied_connection, NM_SETTING_COMPARE_FLAG_NONE)) - nm_connection_clear_secrets ((NMConnection *) priv->settings_connection); + nm_connection_clear_secrets ((NMConnection *) priv->settings_connection.obj); nm_connection_clear_secrets (priv->applied_connection); } @@ -856,11 +848,11 @@ _set_activation_type (NMActiveConnection *self, priv->activation_type = activation_type; - if (priv->settings_connection) { + if (priv->settings_connection.obj) { if (activation_type == NM_ACTIVATION_TYPE_EXTERNAL) - g_signal_connect (priv->settings_connection, "notify::"NM_SETTINGS_CONNECTION_FLAGS, (GCallback) _settings_connection_notify_flags, self); + g_signal_connect (priv->settings_connection.obj, "notify::"NM_SETTINGS_CONNECTION_FLAGS, (GCallback) _settings_connection_notify_flags, self); else - g_signal_handlers_disconnect_by_func (priv->settings_connection, _settings_connection_notify_flags, self); + g_signal_handlers_disconnect_by_func (priv->settings_connection.obj, _settings_connection_notify_flags, self); } } @@ -906,7 +898,7 @@ _settings_connection_notify_flags (NMSettingsConnection *settings_connection, nm_assert (NM_IS_ACTIVE_CONNECTION (self)); nm_assert (NM_IS_SETTINGS_CONNECTION (settings_connection)); nm_assert (nm_active_connection_get_activation_type (self) == NM_ACTIVATION_TYPE_EXTERNAL); - nm_assert (NM_ACTIVE_CONNECTION_GET_PRIVATE (self)->settings_connection == settings_connection); + nm_assert (NM_ACTIVE_CONNECTION_GET_PRIVATE (self)->settings_connection.obj == settings_connection); if (NM_FLAGS_HAS (nm_settings_connection_get_flags (settings_connection), NM_SETTINGS_CONNECTION_FLAGS_NM_GENERATED)) @@ -1077,11 +1069,11 @@ nm_active_connection_authorize (NMActiveConnection *self, if (initial_connection) { g_return_if_fail (NM_IS_CONNECTION (initial_connection)); - g_return_if_fail (!priv->settings_connection); + g_return_if_fail (!priv->settings_connection.obj); g_return_if_fail (!priv->applied_connection); con = initial_connection; } else { - g_return_if_fail (NM_IS_SETTINGS_CONNECTION (priv->settings_connection)); + g_return_if_fail (NM_IS_SETTINGS_CONNECTION (priv->settings_connection.obj)); g_return_if_fail (NM_IS_CONNECTION (priv->applied_connection)); con = priv->applied_connection; } @@ -1169,25 +1161,25 @@ get_property (GObject *object, guint prop_id, switch (prop_id) { - /* note that while priv->settings_connection might not be set initially, + /* note that while priv->settings_connection.obj might not be set initially, * it will be set before the object is exported on D-Bus. Hence, * nobody is calling these property getters before the object is * exported, at which point we will have a valid settings-connection. * - * Therefore, intentionally not check whether priv->settings_connection + * Therefore, intentionally not check whether priv->settings_connection.obj * is set, to get an assertion failure if somebody tries to access the * getters at the wrong time. */ case PROP_CONNECTION: - nm_dbus_utils_g_value_set_object_path_still_exported (value, priv->settings_connection); + g_value_set_string (value, nm_dbus_track_obj_path_get (&priv->settings_connection)); break; case PROP_ID: - g_value_set_string (value, nm_connection_get_id (NM_CONNECTION (priv->settings_connection))); + g_value_set_string (value, nm_connection_get_id (NM_CONNECTION (priv->settings_connection.obj))); break; case PROP_UUID: - g_value_set_string (value, nm_connection_get_uuid (NM_CONNECTION (priv->settings_connection))); + g_value_set_string (value, nm_connection_get_uuid (NM_CONNECTION (priv->settings_connection.obj))); break; case PROP_TYPE: - g_value_set_string (value, nm_connection_get_connection_type (NM_CONNECTION (priv->settings_connection))); + g_value_set_string (value, nm_connection_get_connection_type (NM_CONNECTION (priv->settings_connection.obj))); break; case PROP_SPECIFIC_OBJECT: @@ -1343,6 +1335,10 @@ nm_active_connection_init (NMActiveConnection *self) priv = G_TYPE_INSTANCE_GET_PRIVATE (self, NM_TYPE_ACTIVE_CONNECTION, NMActiveConnectionPrivate); self->_priv = priv; + nm_dbus_track_obj_path_init (&priv->settings_connection, + G_OBJECT (self), + obj_properties[PROP_CONNECTION]); + c_list_init (&self->active_connections_lst); _LOGT ("creating"); @@ -1360,8 +1356,8 @@ constructed (GObject *object) G_OBJECT_CLASS (nm_active_connection_parent_class)->constructed (object); if ( !priv->applied_connection - && priv->settings_connection) - priv->applied_connection = nm_simple_connection_new_clone (NM_CONNECTION (priv->settings_connection)); + && priv->settings_connection.obj) + priv->applied_connection = nm_simple_connection_new_clone (NM_CONNECTION (priv->settings_connection.obj)); _LOGD ("constructed (%s, version-id %llu, type %s)", G_OBJECT_TYPE_NAME (self), @@ -1419,6 +1415,17 @@ dispose (GObject *object) G_OBJECT_CLASS (nm_active_connection_parent_class)->dispose (object); } +static void +finalize (GObject *object) +{ + NMActiveConnection *self = NM_ACTIVE_CONNECTION (object); + NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); + + nm_dbus_track_obj_path_set (&priv->settings_connection, NULL, FALSE); + + G_OBJECT_CLASS (nm_active_connection_parent_class)->finalize (object); +} + static const GDBusSignalInfo signal_info_state_changed = NM_DEFINE_GDBUS_SIGNAL_INFO_INIT ( "StateChanged", .args = NM_DEFINE_GDBUS_ARG_INFOS ( @@ -1471,6 +1478,7 @@ nm_active_connection_class_init (NMActiveConnectionClass *ac_class) object_class->set_property = set_property; object_class->constructed = constructed; object_class->dispose = dispose; + object_class->finalize = finalize; obj_properties[PROP_CONNECTION] = g_param_spec_string (NM_ACTIVE_CONNECTION_CONNECTION, "", "", From be70e716982921b3feda319a10686f62815c6caf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Apr 2018 19:43:54 +0200 Subject: [PATCH 25/40] core: use NMDBusTrackObjPath for NM_DEVICE_PARENT path --- src/devices/nm-device.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 651390db60..14034ee97e 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -256,7 +256,7 @@ typedef struct _NMDevicePrivate { GSList *pending_actions; GSList *dad6_failed_addrs; - NMDevice *parent_device; + NMDBusTrackObjPath parent_device; char * udi; char * iface; /* may change, could be renamed by user */ @@ -1514,12 +1514,9 @@ nm_device_parent_get_ifindex (NMDevice *self) NMDevice * nm_device_parent_get_device (NMDevice *self) { - NMDevicePrivate *priv; - g_return_val_if_fail (NM_IS_DEVICE (self), NULL); - priv = NM_DEVICE_GET_PRIVATE (self); - return priv->parent_device; + return NM_DEVICE_GET_PRIVATE (self)->parent_device.obj; } static void @@ -1541,7 +1538,7 @@ _parent_set_ifindex (NMDevice *self, NMDevice *parent_device; gboolean changed = FALSE; int old_ifindex; - NMDevice *old_device; + gs_unref_object NMDevice *old_device = NULL; g_return_val_if_fail (NM_IS_DEVICE (self), FALSE); @@ -1551,16 +1548,15 @@ _parent_set_ifindex (NMDevice *self, parent_ifindex = 0; old_ifindex = priv->parent_ifindex; - old_device = priv->parent_device; if (priv->parent_ifindex == parent_ifindex) { if (parent_ifindex > 0) { if ( !force_check - && priv->parent_device - && nm_device_get_ifindex (priv->parent_device) == parent_ifindex) + && priv->parent_device.obj + && nm_device_get_ifindex (priv->parent_device.obj) == parent_ifindex) return FALSE; } else { - if (!priv->parent_device) + if (!priv->parent_device.obj) return FALSE; } } else { @@ -1575,24 +1571,23 @@ _parent_set_ifindex (NMDevice *self, } else parent_device = NULL; - if (parent_device != priv->parent_device) { - priv->parent_device = parent_device; + if (parent_device != priv->parent_device.obj) { + old_device = nm_g_object_ref (priv->parent_device.obj); + nm_dbus_track_obj_path_set (&priv->parent_device, parent_device, TRUE); changed = TRUE; } if (changed) { if (priv->parent_ifindex <= 0) _LOGD (LOGD_DEVICE, "parent: clear"); - else if (!priv->parent_device) + else if (!priv->parent_device.obj) _LOGD (LOGD_DEVICE, "parent: ifindex %d, no device", priv->parent_ifindex); else { _LOGD (LOGD_DEVICE, "parent: ifindex %d, device %p, %s", priv->parent_ifindex, - priv->parent_device, nm_device_get_iface (priv->parent_device)); + priv->parent_device.obj, nm_device_get_iface (priv->parent_device.obj)); } - NM_DEVICE_GET_CLASS (self)->parent_changed_notify (self, old_ifindex, old_device, priv->parent_ifindex, priv->parent_device); - - _notify (self, PROP_PARENT); + NM_DEVICE_GET_CLASS (self)->parent_changed_notify (self, old_ifindex, old_device, priv->parent_ifindex, priv->parent_device.obj); } return changed; } @@ -1617,7 +1612,7 @@ nm_device_parent_notify_changed (NMDevice *self, priv = NM_DEVICE_GET_PRIVATE (self); if (priv->parent_ifindex > 0) { - if ( priv->parent_device == change_candidate + if ( priv->parent_device.obj == change_candidate || priv->parent_ifindex == nm_device_get_ifindex (change_candidate)) return _parent_set_ifindex (self, priv->parent_ifindex, device_removed); } @@ -14791,6 +14786,8 @@ nm_device_init (NMDevice *self) priv->connectivity_state = NM_CONNECTIVITY_UNKNOWN; + nm_dbus_track_obj_path_init (&priv->parent_device, G_OBJECT (self), obj_properties[PROP_PARENT]); + priv->netns = g_object_ref (NM_NETNS_GET); priv->autoconnect_blocked_flags = DEFAULT_AUTOCONNECT @@ -15029,6 +15026,8 @@ finalize (GObject *object) g_hash_table_unref (priv->ip6_saved_properties); g_hash_table_unref (priv->available_connections); + nm_dbus_track_obj_path_deinit (&priv->parent_device); + G_OBJECT_CLASS (nm_device_parent_class)->finalize (object); /* for testing, NMDeviceTest does not invoke NMDevice::constructed, @@ -15261,7 +15260,7 @@ get_property (GObject *object, guint prop_id, g_value_set_object (value, nm_device_get_master (self)); break; case PROP_PARENT: - nm_dbus_utils_g_value_set_object_path (value, priv->parent_device); + g_value_set_string (value, nm_dbus_track_obj_path_get (&priv->parent_device)); break; case PROP_HW_ADDRESS: g_value_set_string (value, priv->hw_addr); From bfaa291d89c02ee68780aa28eb7f19e1a925954f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Apr 2018 19:43:54 +0200 Subject: [PATCH 26/40] core: use NMDBusTrackObjPath for NM_DEVICE_ACTIVE_CONNECTION path --- src/devices/nm-device.c | 105 +++++++++++++++++----------------------- 1 file changed, 44 insertions(+), 61 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 14034ee97e..882e9a86d1 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -321,9 +321,7 @@ typedef struct _NMDevicePrivate { NMActRequest * queued_act_request; bool queued_act_request_is_waiting_for_carrier:1; - bool act_request_public:1; - NMActRequest *act_request; - gulong act_request_id; + NMDBusTrackObjPath act_request; ActivationHandleData act_handle4; /* for layer2 and IPv4. */ ActivationHandleData act_handle6; guint recheck_assume_id; @@ -2159,7 +2157,7 @@ nm_device_get_act_request (NMDevice *self) { g_return_val_if_fail (NM_IS_DEVICE (self), NULL); - return NM_DEVICE_GET_PRIVATE (self)->act_request; + return NM_DEVICE_GET_PRIVATE (self)->act_request.obj; } NMSettingsConnection * @@ -2167,7 +2165,7 @@ nm_device_get_settings_connection (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - return priv->act_request ? nm_act_request_get_settings_connection (priv->act_request) : NULL; + return priv->act_request.obj ? nm_act_request_get_settings_connection (priv->act_request.obj) : NULL; } NMConnection * @@ -2179,7 +2177,7 @@ nm_device_get_applied_connection (NMDevice *self) priv = NM_DEVICE_GET_PRIVATE (self); - return priv->act_request ? nm_act_request_get_applied_connection (priv->act_request) : NULL; + return priv->act_request.obj ? nm_act_request_get_applied_connection (priv->act_request.obj) : NULL; } gboolean @@ -2187,10 +2185,10 @@ nm_device_has_unmodified_applied_connection (NMDevice *self, NMSettingCompareFla { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - if (!priv->act_request) + if (!priv->act_request.obj) return FALSE; - return nm_active_connection_has_unmodified_applied_connection ((NMActiveConnection *) priv->act_request, compare_flags); + return nm_active_connection_has_unmodified_applied_connection ((NMActiveConnection *) priv->act_request.obj, compare_flags); } NMSetting * @@ -4528,7 +4526,7 @@ check_ip_state (NMDevice *self, gboolean may_fail, gboolean full_state_update) /* Don't progress into IP_CHECK or SECONDARIES if we're waiting for the * master to enslave us. */ - if ( nm_active_connection_get_master (NM_ACTIVE_CONNECTION (priv->act_request)) + if ( nm_active_connection_get_master (NM_ACTIVE_CONNECTION (priv->act_request.obj)) && !priv->is_enslaved) return; @@ -5721,8 +5719,9 @@ activate_stage1_device_prepare (NMDevice *self) _set_ip_state (self, AF_INET6, IP_NONE); /* Notify the new ActiveConnection along with the state change */ - priv->act_request_public = TRUE; - _notify (self, PROP_ACTIVE_CONNECTION); + nm_dbus_track_obj_path_set (&priv->act_request, + priv->act_request.obj, + TRUE); nm_device_state_changed (self, NM_DEVICE_STATE_PREPARE, NM_DEVICE_STATE_REASON_NONE); @@ -5758,7 +5757,7 @@ nm_device_activate_schedule_stage1_device_prepare (NMDevice *self) g_return_if_fail (NM_IS_DEVICE (self)); priv = NM_DEVICE_GET_PRIVATE (self); - g_return_if_fail (priv->act_request); + g_return_if_fail (priv->act_request.obj); activation_source_schedule (self, activate_stage1_device_prepare, AF_INET); } @@ -5935,7 +5934,7 @@ activate_stage2_device_config (NMDevice *self) if (slave_state == NM_DEVICE_STATE_IP_CONFIG) nm_device_master_enslave_slave (self, info->slave, nm_device_get_applied_connection (info->slave)); - else if ( priv->act_request + else if ( priv->act_request.obj && nm_device_sys_iface_state_is_external (self) && slave_state <= NM_DEVICE_STATE_DISCONNECTED) nm_device_queue_recheck_assume (info->slave); @@ -5960,10 +5959,10 @@ nm_device_activate_schedule_stage2_device_config (NMDevice *self) g_return_if_fail (NM_IS_DEVICE (self)); priv = NM_DEVICE_GET_PRIVATE (self); - g_return_if_fail (priv->act_request); + g_return_if_fail (priv->act_request.obj); if (!priv->master_ready_handled) { - NMActiveConnection *active = NM_ACTIVE_CONNECTION (priv->act_request); + NMActiveConnection *active = NM_ACTIVE_CONNECTION (priv->act_request.obj); NMActiveConnection *master; master = nm_active_connection_get_master (active); @@ -6251,10 +6250,10 @@ nm_device_handle_ipv4ll_event (sd_ipv4ll *ll, int event, void *data) NMIP4Config *config; int r; - if (priv->act_request == NULL) + if (priv->act_request.obj == NULL) return; - connection = nm_act_request_get_applied_connection (priv->act_request); + connection = nm_act_request_get_applied_connection (priv->act_request.obj); g_assert (connection); /* Ignore if the connection isn't an AutoIP connection */ @@ -8174,7 +8173,7 @@ ndisc_config_changed (NMNDisc *ndisc, const NMNDiscData *rdata, guint changed_in NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); guint i; - g_return_if_fail (priv->act_request); + g_return_if_fail (priv->act_request.obj); if (!applied_config_get_current (&priv->ac_ip6_config)) applied_config_init_new (&priv->ac_ip6_config, self, AF_INET6); @@ -8986,7 +8985,7 @@ nm_device_activate_schedule_stage3_ip_config_start (NMDevice *self) g_return_if_fail (NM_IS_DEVICE (self)); priv = NM_DEVICE_GET_PRIVATE (self); - g_return_if_fail (priv->act_request); + g_return_if_fail (priv->act_request.obj); /* Add the interface to the specified firewall zone */ if (priv->fw_state == FIREWALL_STATE_UNMANAGED) { @@ -9058,7 +9057,7 @@ nm_device_activate_schedule_ip4_config_timeout (NMDevice *self) g_return_if_fail (NM_IS_DEVICE (self)); priv = NM_DEVICE_GET_PRIVATE (self); - g_return_if_fail (priv->act_request); + g_return_if_fail (priv->act_request.obj); activation_source_schedule (self, activate_stage4_ip4_config_timeout, AF_INET); } @@ -9114,7 +9113,7 @@ nm_device_activate_schedule_ip6_config_timeout (NMDevice *self) g_return_if_fail (NM_IS_DEVICE (self)); priv = NM_DEVICE_GET_PRIVATE (self); - g_return_if_fail (priv->act_request); + g_return_if_fail (priv->act_request.obj); activation_source_schedule (self, activate_stage4_ip6_config_timeout, AF_INET6); } @@ -9571,43 +9570,27 @@ nm_device_activate_ip6_state_done (NMDevice *self) /*****************************************************************************/ -static void -act_request_exported_changed (NMActRequest *act_request, - NMDevice *self) -{ - _notify (self, PROP_ACTIVE_CONNECTION); -} - static void act_request_set (NMDevice *self, NMActRequest *act_request) { NMDevicePrivate *priv; - gs_unref_object NMActRequest *old_act_requst = NULL; nm_assert (NM_IS_DEVICE (self)); nm_assert (!act_request || NM_IS_ACT_REQUEST (act_request)); priv = NM_DEVICE_GET_PRIVATE (self); - if ( !priv->act_request_public - && priv->act_request == act_request) + if ( !priv->act_request.visible + && priv->act_request.obj == act_request) return; /* always clear the public flag. The few callers that set a new @act_request * don't want that the property is public yet. */ - priv->act_request_public = FALSE; - - nm_clear_g_signal_handler (priv->act_request, &priv->act_request_id); - - old_act_requst = priv->act_request; - priv->act_request = nm_g_object_ref (act_request); + nm_dbus_track_obj_path_set (&priv->act_request, + act_request, + FALSE); if (act_request) { - priv->act_request_id = g_signal_connect (act_request, - NM_DBUS_OBJECT_EXPORTED_CHANGED, - G_CALLBACK (act_request_exported_changed), - self); - switch (nm_active_connection_get_activation_type (NM_ACTIVE_CONNECTION (act_request))) { case NM_ACTIVATION_TYPE_EXTERNAL: break; @@ -9624,8 +9607,6 @@ act_request_set (NMDevice *self, NMActRequest *act_request) break; } } - - _notify (self, PROP_ACTIVE_CONNECTION); } static void @@ -10090,7 +10071,7 @@ check_and_reapply_connection (NMDevice *self, } if ( version_id != 0 - && version_id != nm_active_connection_version_id_get ((NMActiveConnection *) priv->act_request)) { + && version_id != nm_active_connection_version_id_get ((NMActiveConnection *) priv->act_request.obj)) { g_set_error_literal (error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_VERSION_ID_MISMATCH, @@ -10103,10 +10084,10 @@ check_and_reapply_connection (NMDevice *self, *************************************************************************/ if (diffs) - nm_active_connection_version_id_bump ((NMActiveConnection *) priv->act_request); + nm_active_connection_version_id_bump ((NMActiveConnection *) priv->act_request.obj); _LOGD (LOGD_DEVICE, "reapply (version-id %llu%s)", - (unsigned long long) nm_active_connection_version_id_get (((NMActiveConnection *) priv->act_request)), + (unsigned long long) nm_active_connection_version_id_get (((NMActiveConnection *) priv->act_request.obj)), diffs ? "" : " (unmodified)"); if (diffs) { @@ -10356,7 +10337,7 @@ get_applied_connection_cb (NMDevice *self, g_dbus_method_invocation_return_value (context, g_variant_new ("(@a{sa{sv}}t)", settings, - nm_active_connection_version_id_get ((NMActiveConnection *) priv->act_request))); + nm_active_connection_version_id_get ((NMActiveConnection *) priv->act_request.obj))); } static void @@ -10554,7 +10535,7 @@ impl_device_disconnect (NMDBusObject *obj, NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); NMConnection *connection; - if (!priv->act_request) { + if (!priv->act_request.obj) { g_dbus_method_invocation_return_error_literal (invocation, NM_DEVICE_ERROR, NM_DEVICE_ERROR_NOT_ACTIVE, @@ -10742,8 +10723,8 @@ nm_device_steal_connection (NMDevice *self, NMSettingsConnection *connection) && connection == nm_active_connection_get_settings_connection (NM_ACTIVE_CONNECTION (priv->queued_act_request))) _clear_queued_act_request (priv); - if ( priv->act_request - && connection == nm_active_connection_get_settings_connection (NM_ACTIVE_CONNECTION (priv->act_request)) + if ( priv->act_request.obj + && connection == nm_active_connection_get_settings_connection (NM_ACTIVE_CONNECTION (priv->act_request.obj)) && priv->state < NM_DEVICE_STATE_DEACTIVATING) { nm_device_state_changed (self, NM_DEVICE_STATE_DEACTIVATING, @@ -10759,7 +10740,7 @@ nm_device_queue_activation (NMDevice *self, NMActRequest *req) must_queue = _carrier_wait_check_act_request_must_queue (self, req); - if ( !priv->act_request + if ( !priv->act_request.obj && !must_queue && nm_device_is_real (self)) { _device_activate (self, req); @@ -10774,7 +10755,7 @@ nm_device_queue_activation (NMDevice *self, NMActRequest *req) _LOGD (LOGD_DEVICE, "queue activation request waiting for %s", must_queue ? "carrier" : "currently active connection to disconnect"); /* Deactivate existing activation request first */ - if (priv->act_request) { + if (priv->act_request.obj) { _LOGI (LOGD_DEVICE, "disconnecting for new activation request."); nm_device_state_changed (self, NM_DEVICE_STATE_DEACTIVATING, @@ -10989,7 +10970,7 @@ nm_device_set_ip_config (NMDevice *self, && (settings_connection = nm_device_get_settings_connection (self)) && NM_FLAGS_HAS (nm_settings_connection_get_flags (settings_connection), NM_SETTINGS_CONNECTION_FLAGS_NM_GENERATED) - && nm_active_connection_get_activation_type (NM_ACTIVE_CONNECTION (priv->act_request)) == NM_ACTIVATION_TYPE_EXTERNAL) { + && nm_active_connection_get_activation_type (NM_ACTIVE_CONNECTION (priv->act_request.obj)) == NM_ACTIVATION_TYPE_EXTERNAL) { g_object_freeze_notify (G_OBJECT (settings_connection)); nm_connection_add_setting (NM_CONNECTION (settings_connection), IS_IPv4 @@ -12464,7 +12445,7 @@ nm_device_reapply_settings_immediately (NMDevice *self) if (g_strcmp0 ((zone = nm_setting_connection_get_zone (s_con_settings)), nm_setting_connection_get_zone (s_con_applied)) != 0) { - version_id = nm_active_connection_version_id_bump ((NMActiveConnection *) self->_priv->act_request); + version_id = nm_active_connection_version_id_bump ((NMActiveConnection *) self->_priv->act_request.obj); _LOGD (LOGD_DEVICE, "reapply setting: zone = %s%s%s (version-id %llu)", NM_PRINT_FMT_QUOTE_STRING (zone), (unsigned long long) version_id); g_object_set (G_OBJECT (s_con_applied), @@ -12476,7 +12457,7 @@ nm_device_reapply_settings_immediately (NMDevice *self) if ((metered = nm_setting_connection_get_metered (s_con_settings)) != nm_setting_connection_get_metered (s_con_applied)) { - version_id = nm_active_connection_version_id_bump ((NMActiveConnection *) self->_priv->act_request); + version_id = nm_active_connection_version_id_bump ((NMActiveConnection *) self->_priv->act_request.obj); _LOGD (LOGD_DEVICE, "reapply setting: metered = %d (version-id %llu)", (int) metered, (unsigned long long) version_id); g_object_set (G_OBJECT (s_con_applied), @@ -13082,11 +13063,11 @@ _cleanup_generic_post (NMDevice *self, CleanupType cleanup_type) * above disables them. */ nm_assert (priv->needs_ip6_subnet == FALSE); - if (priv->act_request) { - nm_active_connection_set_default (NM_ACTIVE_CONNECTION (priv->act_request), AF_INET, FALSE); + if (priv->act_request.obj) { + nm_active_connection_set_default (NM_ACTIVE_CONNECTION (priv->act_request.obj), AF_INET, FALSE); priv->master_ready_handled = FALSE; - nm_clear_g_signal_handler (priv->act_request, &priv->master_ready_id); + nm_clear_g_signal_handler (priv->act_request.obj, &priv->master_ready_id); act_request_set (self, NULL); } @@ -13538,7 +13519,7 @@ _set_state_full (NMDevice *self, g_cancellable_cancel (priv->deactivating_cancellable); /* Cache the activation request for the dispatcher */ - req = nm_g_object_ref (priv->act_request); + req = nm_g_object_ref (priv->act_request.obj); if ( state > NM_DEVICE_STATE_UNMANAGED && state <= NM_DEVICE_STATE_ACTIVATED @@ -14787,6 +14768,7 @@ nm_device_init (NMDevice *self) priv->connectivity_state = NM_CONNECTIVITY_UNKNOWN; nm_dbus_track_obj_path_init (&priv->parent_device, G_OBJECT (self), obj_properties[PROP_PARENT]); + nm_dbus_track_obj_path_init (&priv->act_request, G_OBJECT (self), obj_properties[PROP_ACTIVE_CONNECTION]); priv->netns = g_object_ref (NM_NETNS_GET); @@ -15027,6 +15009,7 @@ finalize (GObject *object) g_hash_table_unref (priv->available_connections); nm_dbus_track_obj_path_deinit (&priv->parent_device); + nm_dbus_track_obj_path_deinit (&priv->act_request); G_OBJECT_CLASS (nm_device_parent_class)->finalize (object); @@ -15218,7 +15201,7 @@ get_property (GObject *object, guint prop_id, g_variant_new ("(uu)", priv->state, priv->state_reason)); break; case PROP_ACTIVE_CONNECTION: - nm_dbus_utils_g_value_set_object_path_still_exported (value, priv->act_request_public ? priv->act_request : NULL); + g_value_set_string (value, nm_dbus_track_obj_path_get (&priv->act_request)); break; case PROP_DEVICE_TYPE: g_value_set_uint (value, priv->type); From 50b74731f668067d445a28d7e545a169bf9d25c8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 11:52:55 +0200 Subject: [PATCH 27/40] auth-chain/trivial: rename nm_auth_chain_unref() to nm_auth_chain_destroy() NMAuthChain is not really ref-counted. True, we have an internal ref-counter to ensure that the instance stays alive while the callback is invoked. However, the user cannot take additional references as there is no nm_auth_chain_ref(). When the user wants to get rid of the auth-chain, with the current API it is important that the callback won't be called after that point. From the name nm_auth_chain_unref(), it sounds like that there could be multiple references to the auth-chain, and merely unreferencing the object might not guarantee that the callback is canceled. However, that is luckily not the case, because there is no real ref-counting involved here. Just rename the destroy function to make this clearer. --- src/nm-active-connection.c | 4 ++-- src/nm-auth-utils.c | 13 ++++++++----- src/nm-auth-utils.h | 2 +- src/nm-manager.c | 20 ++++++++++---------- src/settings/nm-agent-manager.c | 12 ++++++------ src/settings/nm-settings-connection.c | 4 ++-- src/settings/nm-settings.c | 6 +++--- 7 files changed, 32 insertions(+), 29 deletions(-) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index e182e65df9..a02deb3b5b 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -1030,7 +1030,7 @@ auth_done (NMAuthChain *chain, priv->result_func (self, TRUE, NULL, priv->user_data1, priv->user_data2); done: - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); priv->chain = NULL; priv->result_func = NULL; priv->user_data1 = NULL; @@ -1388,7 +1388,7 @@ dispose (GObject *object) _LOGD ("disposing"); if (priv->chain) { - nm_auth_chain_unref (priv->chain); + nm_auth_chain_destroy (priv->chain); priv->chain = NULL; } diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 93aee45008..da5843b0bf 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -219,7 +219,7 @@ auth_chain_finish (gpointer user_data) /* Ensure we stay alive across the callback */ self->refcount++; self->done_func (self, NULL, self->context, self->user_data); - nm_auth_chain_unref (self); + nm_auth_chain_destroy (self); return FALSE; } @@ -394,17 +394,20 @@ nm_auth_chain_new_subject (NMAuthSubject *subject, } /** - * nm_auth_chain_unref: + * nm_auth_chain_destroy: * @self: the auth-chain * - * Unrefs the auth-chain. By unrefing the auth-chain, you also cancel + * Destroys the auth-chain. By destroying the auth-chain, you also cancel * the receipt of the done-callback. IOW, the callback will not be invoked. * - * The only exception is, if you call nm_auth_chain_unref() from inside + * The only exception is, if may call nm_auth_chain_destroy() from inside * the callback. In this case, @self stays alive until the callback returns. + * + * Note that you might only destroy an auth-chain exactly once, and never + * after the callback was handled. */ void -nm_auth_chain_unref (NMAuthChain *self) +nm_auth_chain_destroy (NMAuthChain *self) { AuthCall *call; diff --git a/src/nm-auth-utils.h b/src/nm-auth-utils.h index 335edb5b36..808da86a51 100644 --- a/src/nm-auth-utils.h +++ b/src/nm-auth-utils.h @@ -62,7 +62,7 @@ void nm_auth_chain_add_call (NMAuthChain *chain, const char *permission, gboolean allow_interaction); -void nm_auth_chain_unref (NMAuthChain *chain); +void nm_auth_chain_destroy (NMAuthChain *chain); /* Caller must free returned error description */ gboolean nm_auth_is_subject_in_acl (NMConnection *connection, diff --git a/src/nm-manager.c b/src/nm-manager.c index ed1aace333..612f41562c 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -953,7 +953,7 @@ _reload_auth_cb (NMAuthChain *chain, g_dbus_method_invocation_return_value (context, NULL); out: - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static void @@ -2100,7 +2100,7 @@ device_auth_done_cb (NMAuthChain *chain, nm_auth_chain_get_data (chain, "user-data")); g_clear_error (&error); - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static void @@ -4796,7 +4796,7 @@ deactivate_net_auth_done_cb (NMAuthChain *chain, else g_dbus_method_invocation_return_value (context, NULL); - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static void @@ -5138,7 +5138,7 @@ sleep_auth_done_cb (NMAuthChain *chain, g_dbus_method_invocation_return_value (context, NULL); } - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } #endif @@ -5276,7 +5276,7 @@ enable_net_done_cb (NMAuthChain *chain, g_dbus_method_invocation_take_error (context, ret_error); } - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static void @@ -5384,7 +5384,7 @@ get_permissions_done_cb (NMAuthChain *chain, g_variant_new ("(a{ss})", &results)); } - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static void @@ -5606,7 +5606,7 @@ check_connectivity_auth_done_cb (NMAuthChain *chain, } out: - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static void @@ -6017,7 +6017,7 @@ out: g_dbus_method_invocation_return_dbus_error (invocation, error_name, error_message); else g_dbus_method_invocation_return_value (invocation, NULL); - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } void @@ -6152,7 +6152,7 @@ checkpoint_auth_done_cb (NMAuthChain *chain, else g_dbus_method_invocation_return_value (context, variant); - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static void @@ -6830,7 +6830,7 @@ dispose (GObject *object) g_slice_free (PlatformLinkCbData, data); } - g_slist_free_full (priv->auth_chains, (GDestroyNotify) nm_auth_chain_unref); + g_slist_free_full (priv->auth_chains, (GDestroyNotify) nm_auth_chain_destroy); priv->auth_chains = NULL; nm_clear_g_source (&priv->devices_inited_id); diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index b2cdc7d922..00aa3a0865 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -360,7 +360,7 @@ agent_register_permissions_done (NMAuthChain *chain, request_add_agent (c_list_entry (iter, Request, lst_request), agent); } - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static NMSecretAgent * @@ -539,7 +539,7 @@ request_free (Request *req) g_object_unref (req->con.connection); g_free (req->con.path); if (req->con.chain) - nm_auth_chain_unref (req->con.chain); + nm_auth_chain_destroy (req->con.chain); if (req->request_type == REQUEST_TYPE_CON_GET) { g_free (req->con.get.setting_name); g_strfreev (req->con.get.hints); @@ -810,7 +810,7 @@ request_remove_agent (Request *req, NMSecretAgent *agent) case REQUEST_TYPE_CON_DEL: if (req->con.chain) { /* This cancels the pending authorization requests. */ - nm_auth_chain_unref (req->con.chain); + nm_auth_chain_destroy (req->con.chain); req->con.chain = NULL; } break; @@ -1047,7 +1047,7 @@ _con_get_request_start_validated (NMAuthChain *chain, _con_get_request_start_proceed (req, req->con.current_has_modify); } - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static void @@ -1541,7 +1541,7 @@ agent_permissions_changed_done (NMAuthChain *chain, nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, share_protected); nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, share_open); - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static void @@ -1616,7 +1616,7 @@ cancel_more: goto cancel_more; } - g_slist_free_full (priv->chains, (GDestroyNotify) nm_auth_chain_unref); + g_slist_free_full (priv->chains, (GDestroyNotify) nm_auth_chain_destroy); priv->chains = NULL; if (priv->agents) { diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 88f9c18085..e72a623e7b 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -1408,7 +1408,7 @@ pk_auth_cb (NMAuthChain *chain, callback (self, context, subject, error, callback_data); g_clear_error (&error); - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } /** @@ -3024,7 +3024,7 @@ dispose (GObject *object) g_clear_object (&priv->agent_secrets); /* Cancel PolicyKit requests */ - g_slist_free_full (priv->pending_auths, (GDestroyNotify) nm_auth_chain_unref); + g_slist_free_full (priv->pending_auths, (GDestroyNotify) nm_auth_chain_destroy); priv->pending_auths = NULL; g_clear_pointer (&priv->seen_bssids, g_hash_table_destroy); diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 67e035f038..20346929d6 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1205,7 +1205,7 @@ pk_add_cb (NMAuthChain *chain, send_agent_owned_secrets (self, added, subject); g_clear_error (&error); - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } /* FIXME: remove if/when kernel supports adhoc wpa */ @@ -1563,7 +1563,7 @@ pk_hostname_cb (NMAuthChain *chain, else g_dbus_method_invocation_return_value (context, NULL); - nm_auth_chain_unref (chain); + nm_auth_chain_destroy (chain); } static void @@ -1912,7 +1912,7 @@ dispose (GObject *object) NMSettings *self = NM_SETTINGS (object); NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); - g_slist_free_full (priv->auths, (GDestroyNotify) nm_auth_chain_unref); + g_slist_free_full (priv->auths, (GDestroyNotify) nm_auth_chain_destroy); priv->auths = NULL; g_object_unref (priv->agent_mgr); From 290d02536c237cd14f187611e72e5fc0b52c81a7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 12:36:06 +0200 Subject: [PATCH 28/40] auth-chain: avoid another idle-call when auth-request completes NMAuthChain schedules (possibly) multiple authentication requests. When they all complete, it will once invoke the result-callback. There is no need to schedule this result-callback on another idle-handler, because nm_auth_manager_polkit_authority_check_authorization() should guarantee to invoke the callback never-synchronously and on a clean call-stack (to avoid problems with re-entrancy). At that point, NMAuthChain does not need to delay this further. --- src/nm-auth-manager.c | 3 +++ src/nm-auth-utils.c | 29 ++++++++++++----------------- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index 003d997505..d1b8d91e1d 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -240,6 +240,9 @@ _call_check_authorization (CheckAuthData *data) data->dbus_parameters = NULL; } +/* + * @callback must never be invoked synchronously. + */ void nm_auth_manager_polkit_authority_check_authorization (NMAuthManager *self, NMAuthSubject *subject, diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index da5843b0bf..3c5440eb2b 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -44,8 +44,6 @@ struct NMAuthChain { NMAuthChainResultFunc done_func; gpointer user_data; - guint idle_id; - guint32 refcount; bool done:1; @@ -209,16 +207,15 @@ nm_auth_chain_get_subject (NMAuthChain *self) /*****************************************************************************/ static gboolean -auth_chain_finish (gpointer user_data) +auth_chain_finish (NMAuthChain *self) { - NMAuthChain *self = user_data; - - self->idle_id = 0; self->done = TRUE; /* Ensure we stay alive across the callback */ + nm_assert (self->refcount == 1); self->refcount++; self->done_func (self, NULL, self->context, self->user_data); + nm_assert (NM_IN_SET (self->refcount, 1, 2)); nm_auth_chain_destroy (self); return FALSE; } @@ -232,14 +229,14 @@ auth_call_complete (AuthCall *call) self = call->chain; - c_list_unlink (&call->auth_call_lst); - - if (c_list_is_empty (&self->auth_call_lst_head)) { - nm_assert (!self->idle_id && !self->done); - self->idle_id = g_idle_add (auth_chain_finish, self); - } + nm_assert (!self->done); auth_call_free (call); + + if (c_list_is_empty (&self->auth_call_lst_head)) { + /* we are on an idle-handler or a clean call-stack (non-reentrant). */ + auth_chain_finish (self); + } } static gboolean @@ -307,10 +304,10 @@ nm_auth_chain_add_call (NMAuthChain *self, NMAuthManager *auth_manager = nm_auth_manager_get (); g_return_if_fail (self); - g_return_if_fail (permission && *permission); g_return_if_fail (self->subject); + g_return_if_fail (!self->done); + g_return_if_fail (permission && *permission); g_return_if_fail (nm_auth_subject_is_unix_process (self->subject) || nm_auth_subject_is_internal (self->subject)); - g_return_if_fail (!self->idle_id && !self->done); call = g_slice_new0 (AuthCall); call->chain = self; @@ -412,13 +409,11 @@ nm_auth_chain_destroy (NMAuthChain *self) AuthCall *call; g_return_if_fail (self); - g_return_if_fail (self->refcount > 0); + g_return_if_fail (NM_IN_SET (self->refcount, 1, 2)); if (--self->refcount > 0) return; - nm_clear_g_source (&self->idle_id); - nm_clear_g_object (&self->subject); nm_clear_g_object (&self->context); From f8b74e19ea1e01781d22fccc6e96129d88e6814e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 12:56:56 +0200 Subject: [PATCH 29/40] auth-manager: emit signal by ID It's more efficient, as it saves a lookup by name. Also, it's more idiomatic to do it this way. I didn't find where the signal gets emitted at first, because usually we don't emit by name. --- src/nm-auth-manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index d1b8d91e1d..441c971f56 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -353,7 +353,7 @@ static void _emit_changed_signal (NMAuthManager *self) { _LOGD ("emit changed signal"); - g_signal_emit_by_name (self, NM_AUTH_MANAGER_SIGNAL_CHANGED); + g_signal_emit (self, signals[CHANGED_SIGNAL], 0); } static void From 999594a56f86cfd2f6383bd223c03445b0e2e561 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 12:58:59 +0200 Subject: [PATCH 30/40] auth-manager: drop unused property getter for NM_AUTH_MANAGER_POLKIT_ENABLED We need the setter, because we want that the property is set only once during creation of the instance. Nobody cares about the GObject property getter otherwise. --- src/nm-auth-manager.c | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index 441c971f56..26bdc3174b 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -44,13 +44,13 @@ enum { static guint signals[LAST_SIGNAL] = {0}; typedef struct { - gboolean polkit_enabled; #if WITH_POLKIT guint call_id_counter; GCancellable *new_proxy_cancellable; GSList *queued_calls; GDBusProxy *proxy; #endif + bool polkit_enabled:1; } NMAuthManagerPrivate; struct _NMAuthManager { @@ -490,21 +490,6 @@ nm_auth_manager_get () /*****************************************************************************/ -static void -get_property (GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) -{ - NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE ((NMAuthManager *) object); - - switch (prop_id) { - case PROP_POLKIT_ENABLED: - g_value_set_boolean (value, priv->polkit_enabled); - break; - default: - G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); - break; - } -} - static void set_property (GObject *object, guint prop_id, const GValue *value, GParamSpec *pspec) { @@ -614,7 +599,6 @@ nm_auth_manager_class_init (NMAuthManagerClass *klass) { GObjectClass *object_class = G_OBJECT_CLASS (klass); - object_class->get_property = get_property; object_class->set_property = set_property; object_class->constructed = constructed; object_class->dispose = dispose; @@ -622,7 +606,7 @@ nm_auth_manager_class_init (NMAuthManagerClass *klass) obj_properties[PROP_POLKIT_ENABLED] = g_param_spec_boolean (NM_AUTH_MANAGER_POLKIT_ENABLED, "", "", FALSE, - G_PARAM_READWRITE | + G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS); From 2ea2df3184d45567fa9c44f5ef90634a779bfb75 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 13:27:03 +0200 Subject: [PATCH 31/40] auth-manager: rework auth-manager's API Don't use the GAsyncResult pattern for internal API of auth-manager. Instead, use a simpler API that has a more strict API and simpler use. - return a call-id handle when scheduling the authorization request. The request is always scheduled asynchronsously and thus call-id is never %NULL. - the call-id can be used to cancel the request. It can be used exactly once, and only before the callback is invoked. - the async keeps the auth-manager alive. It needs to do so, because when cancelling the request we might not yet be done: instead we might still need to issue a CancelCheckAuthorization call (which we need to handle as well). - the callback is always invoked exactly once. Currently NMAuthManager's API effectivly is only called by NMAuthChain. The point of this is to make NMAuthManager's API more consumable, and thus let users use it directly (instead of using the NMAuthChain layer). As well known, we don't do a good job during shutdown of NetworkManager to release all resources and cancel pending requests. This rework also makes it possible to actually get this right. See the comment in nm_auth_manager_force_shutdown(). But yes, it is still a bit complicated to do a controlled shutdown, because we cannot just synchronously complete. We need to issue CancelCheckAuthorization D-Bus calls, and give these requests time to complete. The new API introduced by this patch would make that easier. --- src/nm-auth-manager.c | 543 ++++++++++++++++++++++++------------------ src/nm-auth-manager.h | 33 +-- src/nm-auth-utils.c | 46 ++-- 3 files changed, 351 insertions(+), 271 deletions(-) diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index 26bdc3174b..784a8f23f1 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -22,6 +22,7 @@ #include "nm-auth-manager.h" +#include "nm-utils/c-list.h" #include "nm-errors.h" #include "nm-core-internal.h" #include "NetworkManagerUtils.h" @@ -30,6 +31,9 @@ #define POLKIT_OBJECT_PATH "/org/freedesktop/PolicyKit1/Authority" #define POLKIT_INTERFACE "org.freedesktop.PolicyKit1.Authority" +#define CANCELLATION_ID_PREFIX "cancellation-id-" +#define CANCELLATION_TIMEOUT_MS 5000 + /*****************************************************************************/ NM_GOBJECT_PROPERTIES_DEFINE_BASE ( @@ -45,12 +49,15 @@ static guint signals[LAST_SIGNAL] = {0}; typedef struct { #if WITH_POLKIT - guint call_id_counter; - GCancellable *new_proxy_cancellable; - GSList *queued_calls; + CList calls_lst_head; GDBusProxy *proxy; + GCancellable *new_proxy_cancellable; + GCancellable *cancel_cancellable; + guint64 call_numid_counter; #endif bool polkit_enabled:1; + bool disposing:1; + bool shutting_down:1; } NMAuthManagerPrivate; struct _NMAuthManager { @@ -85,6 +92,22 @@ NM_DEFINE_SINGLETON_REGISTER (NMAuthManager); } \ } G_STMT_END +#define _NMLOG2(level, call_id, ...) \ + G_STMT_START { \ + if (nm_logging_enabled ((level), (_NMLOG_DOMAIN))) { \ + NMAuthManagerCallId *_call_id = (call_id); \ + char __prefix[30] = _NMLOG_PREFIX_NAME; \ + \ + if (_call_id->self != singleton_instance) \ + g_snprintf (__prefix, sizeof (__prefix), ""_NMLOG_PREFIX_NAME"[%p]", _call_id->self); \ + _nm_log ((level), (_NMLOG_DOMAIN), 0, NULL, NULL, \ + "%s: call[%"G_GUINT64_FORMAT"]: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + __prefix, \ + _call_id->call_numid \ + _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ + } \ + } G_STMT_END + /*****************************************************************************/ gboolean @@ -104,247 +127,286 @@ typedef enum { POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION = (1<<0), } PolkitCheckAuthorizationFlags; -typedef struct { - guint call_id; +struct _NMAuthManagerCallId { + CList calls_lst; NMAuthManager *self; - GSimpleAsyncResult *simple; - gchar *cancellation_id; GVariant *dbus_parameters; - GCancellable *cancellable; -} CheckAuthData; + GCancellable *dbus_cancellable; + NMAuthManagerCheckAuthorizationCallback callback; + gpointer user_data; + guint64 call_numid; + guint idle_id; +}; + +#define cancellation_id_to_str_a(call_numid) \ + nm_sprintf_bufa (NM_STRLEN (CANCELLATION_ID_PREFIX) + 20, \ + CANCELLATION_ID_PREFIX"%"G_GUINT64_FORMAT, \ + (call_numid)) static void -_check_auth_data_free (CheckAuthData *data) +_call_id_free (NMAuthManagerCallId *call_id) { - if (data->dbus_parameters) - g_variant_unref (data->dbus_parameters); - g_object_unref (data->self); - g_object_unref (data->simple); - g_clear_object (&data->cancellable); - g_free (data->cancellation_id); - g_free (data); + c_list_unlink (&call_id->calls_lst); + nm_clear_g_source (&call_id->idle_id); + if (call_id->dbus_parameters) + g_variant_unref (g_steal_pointer (&call_id->dbus_parameters)); + + if (call_id->dbus_cancellable) { + /* we have a pending D-Bus call. We keep the call-id instance alive + * for _call_check_authorize_cb() */ + g_cancellable_cancel (call_id->dbus_cancellable); + return; + } + + g_object_unref (call_id->self); + g_slice_free (NMAuthManagerCallId, call_id); } static void -_call_check_authorization_complete_with_error (CheckAuthData *data, - const char *error_message) +_call_id_invoke_callback (NMAuthManagerCallId *call_id, + gboolean is_authorized, + gboolean is_challenge, + GError *error) { - NMAuthManager *self = data->self; + c_list_unlink (&call_id->calls_lst); - _LOGD ("call[%u]: CheckAuthorization failed due to internal error: %s", data->call_id, error_message); - g_simple_async_result_set_error (data->simple, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_FAILED, - "Authorization check failed: %s", - error_message); - - g_simple_async_result_complete_in_idle (data->simple); - - _check_auth_data_free (data); + call_id->callback (call_id->self, + call_id, + is_authorized, + is_challenge, + error, + call_id->user_data); + _call_id_free (call_id); } static void -cancel_check_authorization_cb (GDBusProxy *proxy, +cancel_check_authorization_cb (GObject *proxy, GAsyncResult *res, gpointer user_data) { - NMAuthManager *self = user_data; - GVariant *value; - GError *error= NULL; + NMAuthManagerCallId *call_id = user_data; + gs_unref_variant GVariant *value = NULL; + gs_free_error GError *error= NULL; - value = g_dbus_proxy_call_finish (proxy, res, &error); - if (value == NULL) { - g_dbus_error_strip_remote_error (error); - _LOGD ("Error cancelling authorization check: %s", error->message); - g_error_free (error); - } else - g_variant_unref (value); + value = g_dbus_proxy_call_finish (G_DBUS_PROXY (proxy), res, &error); + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + _LOG2T (call_id, "cancel request was cancelled"); + else if (error) + _LOG2T (call_id, "cancel request failed: %s", error->message); + else + _LOG2T (call_id, "cancel request succeeded"); - g_object_unref (self); + _call_id_free (call_id); } -typedef struct { - gboolean is_authorized; - gboolean is_challenge; -} CheckAuthorizationResult; - static void -check_authorization_cb (GDBusProxy *proxy, - GAsyncResult *res, - gpointer user_data) +_call_check_authorize_cb (GObject *proxy, + GAsyncResult *res, + gpointer user_data) { - CheckAuthData *data = user_data; - NMAuthManager *self = data->self; - NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - GVariant *value; - GError *error = NULL; + NMAuthManagerCallId *call_id = user_data; + NMAuthManager *self; + NMAuthManagerPrivate *priv; + gs_unref_variant GVariant *value = NULL; + gs_free_error GError *error = NULL; + gboolean is_authorized = FALSE; + gboolean is_challenge = FALSE; - value = _nm_dbus_proxy_call_finish (proxy, res, G_VARIANT_TYPE ("((bba{ss}))"), &error); - if (value == NULL) { - if (data->cancellation_id != NULL && - ( g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED) - && !g_dbus_error_is_remote_error (error))) { - _LOGD ("call[%u]: CheckAuthorization cancelled", data->call_id); - g_dbus_proxy_call (priv->proxy, - "CancelCheckAuthorization", - g_variant_new ("(s)", data->cancellation_id), - G_DBUS_CALL_FLAGS_NONE, - -1, - NULL, /* GCancellable */ - (GAsyncReadyCallback) cancel_check_authorization_cb, - g_object_ref (self)); - } else - _LOGD ("call[%u]: CheckAuthorization failed: %s", data->call_id, error->message); - g_dbus_error_strip_remote_error (error); - g_simple_async_result_set_error (data->simple, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_FAILED, - "Authorization check failed: %s", - error->message); - g_error_free (error); - } else { - CheckAuthorizationResult *result; + /* we need to clear the cancelable, to signal for _call_id_free() that we + * are not in a pending call. + * + * Note how _call_id_free() kept call-id alive, even if the request was + * already cancelled. */ + g_clear_object (&call_id->dbus_cancellable); - result = g_new0 (CheckAuthorizationResult, 1); + self = call_id->self; + priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - g_variant_get (value, - "((bb@a{ss}))", - &result->is_authorized, - &result->is_challenge, - NULL); - g_variant_unref (value); + value = _nm_dbus_proxy_call_finish (G_DBUS_PROXY (proxy), res, G_VARIANT_TYPE ("((bba{ss}))"), &error); + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { + /* call_id was cancelled externally, but _call_id_free() kept call_id + * alive (and it has still the reference on @self. */ - _LOGD ("call[%u]: CheckAuthorization succeeded: (is_authorized=%d, is_challenge=%d)", data->call_id, result->is_authorized, result->is_challenge); - g_simple_async_result_set_op_res_gpointer (data->simple, result, g_free); + if (!priv->cancel_cancellable) { + /* we do a forced shutdown. There is no more time for cancelling... */ + _call_id_free (call_id); + + /* this shouldn't really happen, because: + * _call_check_authorize() only scheduled the D-Bus request at a time when + * cancel_cancellable was still set. It means, somebody called force-shutdown + * after call-id was schedule. + * force-shutdown should only be called after: + * - cancel all pending requests + * - give enough time to cancel the request and schedule a D-Bus call + * to CancelCheckAuthorization (below), before issuing force-shutdown. */ + g_return_if_reached (); + } + + g_dbus_proxy_call (priv->proxy, + "CancelCheckAuthorization", + g_variant_new ("(s)", + cancellation_id_to_str_a (call_id->call_numid)), + G_DBUS_CALL_FLAGS_NONE, + CANCELLATION_TIMEOUT_MS, + priv->cancel_cancellable, + cancel_check_authorization_cb, + call_id); + return; } - g_simple_async_result_complete (data->simple); + if (!error) { + g_variant_get (value, + "((bb@a{ss}))", + &is_authorized, + &is_challenge, + NULL); + _LOG2T (call_id, "completed: authorized=%d, challenge=%d", + is_authorized, is_challenge); + } else + _LOG2T (call_id, "completed: failed: %s", error->message); - _check_auth_data_free (data); + _call_id_invoke_callback (call_id, is_authorized, is_challenge, error); } static void -_call_check_authorization (CheckAuthData *data) +_call_check_authorize (NMAuthManagerCallId *call_id) { - NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (data->self); + NMAuthManager *self = call_id->self; + NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); + + nm_assert (call_id->dbus_parameters); + nm_assert (g_variant_is_floating (call_id->dbus_parameters)); + nm_assert (!call_id->dbus_cancellable); + + call_id->dbus_cancellable = g_cancellable_new (); + + nm_assert (priv->cancel_cancellable); g_dbus_proxy_call (priv->proxy, "CheckAuthorization", - data->dbus_parameters, + g_steal_pointer (&call_id->dbus_parameters), G_DBUS_CALL_FLAGS_NONE, G_MAXINT, /* no timeout */ - data->cancellable, - (GAsyncReadyCallback) check_authorization_cb, - data); - g_clear_object (&data->cancellable); - data->dbus_parameters = NULL; + call_id->dbus_cancellable, + _call_check_authorize_cb, + call_id); +} + +static gboolean +_call_fail_on_idle (gpointer user_data) +{ + NMAuthManagerCallId *call_id = user_data; + gs_free_error GError *error = NULL; + + call_id->idle_id = 0; + g_set_error_literal (&error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, + "failure creating GDBusProxy for authorization request"); + _LOG2T (call_id, "completed: failed due to no D-Bus proxy"); + _call_id_invoke_callback (call_id, FALSE, FALSE, error); + return G_SOURCE_REMOVE; } /* * @callback must never be invoked synchronously. + * + * @callback is always invoked exactly once, and never synchronously. + * You may cancel the invocation with nm_auth_manager_check_authorization_cancel(), + * but: you may only do so exactly once, and only before @callback is + * invoked. Even if you cancel the request, @callback will still be invoked + * (synchronously, during the _cancel() callback). + * + * The request keeps @self alive (it needs to do so, because when cancelling a + * request we might need to do an additional CancelCheckAuthorization call, for + * which @self must be live long enough). */ -void -nm_auth_manager_polkit_authority_check_authorization (NMAuthManager *self, - NMAuthSubject *subject, - const char *action_id, - gboolean allow_user_interaction, - GCancellable *cancellable, - GAsyncReadyCallback callback, - gpointer user_data) +NMAuthManagerCallId * +nm_auth_manager_check_authorization (NMAuthManager *self, + NMAuthSubject *subject, + const char *action_id, + gboolean allow_user_interaction, + NMAuthManagerCheckAuthorizationCallback callback, + gpointer user_data) { NMAuthManagerPrivate *priv; + PolkitCheckAuthorizationFlags flags; char subject_buf[64]; GVariantBuilder builder; - PolkitCheckAuthorizationFlags flags; GVariant *subject_value; GVariant *details_value; - CheckAuthData *data; + NMAuthManagerCallId *call_id; - g_return_if_fail (NM_IS_AUTH_MANAGER (self)); - g_return_if_fail (NM_IS_AUTH_SUBJECT (subject)); - g_return_if_fail (nm_auth_subject_is_unix_process (subject)); - g_return_if_fail (action_id != NULL); - g_return_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable)); + g_return_val_if_fail (NM_IS_AUTH_MANAGER (self), NULL); + g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL); + g_return_val_if_fail (nm_auth_subject_is_unix_process (subject), NULL); + g_return_val_if_fail (action_id, NULL); priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - g_return_if_fail (priv->polkit_enabled); + g_return_val_if_fail (priv->polkit_enabled, NULL); + g_return_val_if_fail (!priv->disposing, NULL); + g_return_val_if_fail (!priv->shutting_down, NULL); flags = allow_user_interaction ? POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION : POLKIT_CHECK_AUTHORIZATION_FLAGS_NONE; - subject_value = nm_auth_subject_unix_process_to_polkit_gvariant (subject); - nm_assert (g_variant_is_floating (subject_value)); + call_id = g_slice_new0 (NMAuthManagerCallId); + call_id->self = g_object_ref (self); + call_id->callback = callback; + call_id->user_data = user_data; + call_id->call_numid = ++priv->call_numid_counter; + c_list_link_tail (&priv->calls_lst_head, &call_id->calls_lst); - /* ((PolkitDetails *)NULL) */ - g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{ss}")); - details_value = g_variant_builder_end (&builder); - - data = g_new0 (CheckAuthData, 1); - data->call_id = ++priv->call_id_counter; - data->self = g_object_ref (self); - data->simple = g_simple_async_result_new (G_OBJECT (self), - callback, - user_data, - nm_auth_manager_polkit_authority_check_authorization); - if (cancellable != NULL) { - data->cancellation_id = g_strdup_printf ("cancellation-id-%u", data->call_id); - data->cancellable = g_object_ref (cancellable); - } - - data->dbus_parameters = g_variant_new ("(@(sa{sv})s@a{ss}us)", - subject_value, - action_id, - details_value, - (guint32) flags, - data->cancellation_id != NULL ? data->cancellation_id : ""); - - if (priv->new_proxy_cancellable) { - _LOGD ("call[%u]: CheckAuthorization(%s), subject=%s (wait for proxy)", data->call_id, action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); - - priv->queued_calls = g_slist_prepend (priv->queued_calls, data); - } else if (!priv->proxy) { - _LOGD ("call[%u]: CheckAuthorization(%s), subject=%s (fails due to invalid DBUS proxy)", data->call_id, action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); - - _call_check_authorization_complete_with_error (data, "invalid DBUS proxy"); + if ( !priv->proxy + && !priv->new_proxy_cancellable) { + _LOG2T (call_id, "CheckAuthorization(%s), subject=%s (failing due to invalid DBUS proxy)", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); + call_id->idle_id = g_idle_add (_call_fail_on_idle, call_id); } else { - _LOGD ("call[%u]: CheckAuthorization(%s), subject=%s", data->call_id, action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); + subject_value = nm_auth_subject_unix_process_to_polkit_gvariant (subject); + nm_assert (g_variant_is_floating (subject_value)); - _call_check_authorization (data); + /* ((PolkitDetails *)NULL) */ + g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{ss}")); + details_value = g_variant_builder_end (&builder); + + call_id->dbus_parameters = g_variant_new ("(@(sa{sv})s@a{ss}us)", + subject_value, + action_id, + details_value, + (guint32) flags, + cancellation_id_to_str_a (call_id->call_numid)); + if (!priv->proxy) { + _LOG2T (call_id, "CheckAuthorization(%s), subject=%s (wait for proxy)", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); + } else { + _LOG2T (call_id, "CheckAuthorization(%s), subject=%s", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); + _call_check_authorize (call_id); + } } + + return call_id; } -gboolean -nm_auth_manager_polkit_authority_check_authorization_finish (NMAuthManager *self, - GAsyncResult *res, - gboolean *out_is_authorized, - gboolean *out_is_challenge, - GError **error) +void +nm_auth_manager_check_authorization_cancel (NMAuthManagerCallId *call_id) { - gboolean success = FALSE; - gboolean is_authorized = FALSE; - gboolean is_challenge = FALSE; + NMAuthManager *self; + gs_free_error GError *error = NULL; - g_return_val_if_fail (NM_IS_AUTH_MANAGER (self), FALSE); - g_return_val_if_fail (G_IS_SIMPLE_ASYNC_RESULT (res), FALSE); - g_return_val_if_fail (error == NULL || *error == NULL, FALSE); + g_return_if_fail (call_id); - if (!g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error)) { - CheckAuthorizationResult *result; + self = call_id->self; - result = g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (res)); - is_authorized = !!result->is_authorized; - is_challenge = !!result->is_challenge; - success = TRUE; - } - g_assert ((success && !error) || (!success || error)); + g_return_if_fail (NM_IS_AUTH_MANAGER (self)); + g_return_if_fail (!c_list_is_empty (&call_id->calls_lst)); - if (out_is_authorized) - *out_is_authorized = is_authorized; - if (out_is_challenge) - *out_is_challenge = is_challenge; - return success; + nm_assert (c_list_contains (&NM_AUTH_MANAGER_GET_PRIVATE (self)->calls_lst_head, &call_id->calls_lst)); + + nm_utils_error_set_cancelled (&error, FALSE, "NMAuthManager"); + _LOG2T (call_id, "completed: failed due to call cancelled"); + _call_id_invoke_callback (call_id, + FALSE, + FALSE, + error); } /*****************************************************************************/ @@ -360,7 +422,7 @@ static void _log_name_owner (NMAuthManager *self, char **out_name_owner) { NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - char *name_owner; + gs_free char *name_owner = NULL; name_owner = g_dbus_proxy_get_name_owner (priv->proxy); if (name_owner) @@ -368,10 +430,7 @@ _log_name_owner (NMAuthManager *self, char **out_name_owner) else _LOGD ("dbus name owner: none"); - if (out_name_owner) - *out_name_owner = name_owner; - else - g_free (name_owner); + NM_SET_OUT (out_name_owner, g_steal_pointer (&name_owner)); } static void @@ -380,20 +439,16 @@ _dbus_on_name_owner_notify_cb (GObject *object, gpointer user_data) { NMAuthManager *self = user_data; - NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - char *name_owner; + gs_free char *name_owner = NULL; - g_return_if_fail (priv->proxy == (void *) object); + nm_assert (NM_AUTH_MANAGER_GET_PRIVATE (self)->proxy == (GDBusProxy *) object); _log_name_owner (self, &name_owner); - if (!name_owner) { /* when the name disappears, we also want to raise a emit signal. * When it appears, we raise one already. */ _emit_changed_signal (self); } - - g_free (name_owner); } static void @@ -401,9 +456,8 @@ _dbus_on_changed_signal_cb (GDBusProxy *proxy, gpointer user_data) { NMAuthManager *self = user_data; - NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - g_return_if_fail (priv->proxy == proxy); + nm_assert (NM_AUTH_MANAGER_GET_PRIVATE (self)->proxy == proxy); _LOGD ("dbus signal: \"Changed\""); _emit_changed_signal (self); @@ -414,49 +468,35 @@ _dbus_new_proxy_cb (GObject *source_object, GAsyncResult *res, gpointer user_data) { - NMAuthManager **p_self = user_data; - NMAuthManager *self = NULL; + NMAuthManager *self; NMAuthManagerPrivate *priv; - GError *error = NULL; + gs_free GError *error = NULL; GDBusProxy *proxy; - CheckAuthData *data; + NMAuthManagerCallId *call_id; proxy = g_dbus_proxy_new_for_bus_finish (res, &error); - if (!*p_self) { - _LOGD ("_dbus_new_proxy_cb(): manager destroyed before callback finished. Abort"); - g_clear_object (&proxy); - g_clear_error (&error); - g_free (p_self); + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; - } - self = *p_self; - g_object_remove_weak_pointer (G_OBJECT (self), (void **)p_self); - g_free (p_self); + self = user_data; priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - g_return_if_fail (priv->new_proxy_cancellable); - g_return_if_fail (!priv->proxy); - + priv->proxy = proxy; g_clear_object (&priv->new_proxy_cancellable); - priv->queued_calls = g_slist_reverse (priv->queued_calls); - - priv->proxy = proxy; if (!priv->proxy) { - _LOGE ("could not get polkit proxy: %s", error->message); - g_clear_error (&error); + _LOGE ("could not create polkit proxy: %s", error->message); - while (priv->queued_calls) { - data = priv->queued_calls->data; - priv->queued_calls = g_slist_remove (priv->queued_calls, data); - - _call_check_authorization_complete_with_error (data, "error creating DBUS proxy"); + while ((call_id = c_list_first_entry (&priv->calls_lst_head, NMAuthManagerCallId, calls_lst))) { + _LOG2T (call_id, "completed: failed due to no D-Bus proxy after startup"); + _call_id_invoke_callback (call_id, FALSE, FALSE, error); } return; } + priv->cancel_cancellable = g_cancellable_new (); + g_signal_connect (priv->proxy, "notify::g-name-owner", G_CALLBACK (_dbus_on_name_owner_notify_cb), @@ -467,15 +507,13 @@ _dbus_new_proxy_cb (GObject *source_object, _log_name_owner (self, NULL); - while (priv->queued_calls) { - data = priv->queued_calls->data; - priv->queued_calls = g_slist_remove (priv->queued_calls, data); - _LOGD ("call[%u]: CheckAuthorization invoke now", data->call_id); - _call_check_authorization (data); + while ((call_id = c_list_first_entry (&priv->calls_lst_head, NMAuthManagerCallId, calls_lst))) { + _LOG2T (call_id, "CheckAuthorization invoke now"); + _call_check_authorize (call_id); } + _emit_changed_signal (self); } - #endif /*****************************************************************************/ @@ -488,6 +526,44 @@ nm_auth_manager_get () return singleton_instance; } +void +nm_auth_manager_force_shutdown (NMAuthManager *self) +{ +#if WITH_POLKIT + NMAuthManagerPrivate *priv; + + g_return_if_fail (NM_IS_AUTH_MANAGER (self)); + + priv = NM_AUTH_MANAGER_GET_PRIVATE (self); + + /* while we have pending requests (NMAuthManagerCallId), the instance + * is kept alive. + * + * Even if the caller cancells all pending call-ids, we still need to keep + * a reference to self, in order to handle pending CancelCheckAuthorization + * requests. + * + * To do a corrdinated shutdown, do the following: + * - cancel all pending NMAuthManagerCallId requests. + * - ensure everybody unrefs the NMAuthManager instance. If by that, the instance + * gets destroyed, the shutdown already completed successfully. + * - Otherwise, the object is kept alive by pending CancelCheckAuthorization requests. + * wait a certain timeout (1 second) for all requests to complete (by watching + * for destruction of NMAuthManager). + * - if that doesn't happen within timeout, issue nm_auth_manager_force_shutdown() and + * wait longer. After that, soon the instance should be destroyed and you + * did a successful shutdown. + * - if the instance was still not destroyed within a short timeout, you leaked + * resources. You cannot properly shutdown. + */ + + priv->shutting_down = TRUE; + nm_clear_g_cancellable (&priv->cancel_cancellable); +#else + g_return_if_fail (NM_IS_AUTH_MANAGER (self)); +#endif +} + /*****************************************************************************/ static void @@ -511,6 +587,11 @@ set_property (GObject *object, guint prop_id, const GValue *value, GParamSpec *p static void nm_auth_manager_init (NMAuthManager *self) { +#if WITH_POLKIT + NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); + + c_list_init (&priv->calls_lst_head); +#endif } static void @@ -525,12 +606,7 @@ constructed (GObject *object) _LOGD ("create auth-manager: polkit %s", priv->polkit_enabled ? "enabled" : "disabled"); if (priv->polkit_enabled) { - NMAuthManager **p_self; - priv->new_proxy_cancellable = g_cancellable_new (); - p_self = g_new (NMAuthManager *, 1); - *p_self = self; - g_object_add_weak_pointer (G_OBJECT (self), (void **) p_self); g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, NULL, @@ -539,7 +615,7 @@ constructed (GObject *object) POLKIT_INTERFACE, priv->new_proxy_cancellable, _dbus_new_proxy_cb, - p_self); + self); } #else if (priv->polkit_enabled) @@ -575,15 +651,18 @@ dispose (GObject *object) NMAuthManager* self = NM_AUTH_MANAGER (object); #if WITH_POLKIT NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); + gs_free_error GError *error_disposing = NULL; #endif _LOGD ("dispose"); #if WITH_POLKIT - /* since we take a reference for each queued call, we don't expect to have any queued calls in dispose() */ - g_assert (!priv->queued_calls); + nm_assert (!c_list_is_empty (&priv->calls_lst_head)); + + priv->disposing = TRUE; nm_clear_g_cancellable (&priv->new_proxy_cancellable); + nm_clear_g_cancellable (&priv->cancel_cancellable); if (priv->proxy) { g_signal_handlers_disconnect_by_data (priv->proxy, self); @@ -615,11 +694,7 @@ nm_auth_manager_class_init (NMAuthManagerClass *klass) signals[CHANGED_SIGNAL] = g_signal_new (NM_AUTH_MANAGER_SIGNAL_CHANGED, NM_TYPE_AUTH_MANAGER, G_SIGNAL_RUN_LAST, - 0, /* class offset */ - NULL, /* accumulator */ - NULL, /* accumulator data */ + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, - G_TYPE_NONE, - 0); + G_TYPE_NONE, 0); } - diff --git a/src/nm-auth-manager.h b/src/nm-auth-manager.h index e66ef78c1b..b84debcd6b 100644 --- a/src/nm-auth-manager.h +++ b/src/nm-auth-manager.h @@ -42,23 +42,28 @@ GType nm_auth_manager_get_type (void); NMAuthManager *nm_auth_manager_setup (gboolean polkit_enabled); NMAuthManager *nm_auth_manager_get (void); +void nm_auth_manager_force_shutdown (NMAuthManager *self); + gboolean nm_auth_manager_get_polkit_enabled (NMAuthManager *self); -#if WITH_POLKIT +/*****************************************************************************/ -void nm_auth_manager_polkit_authority_check_authorization (NMAuthManager *self, - NMAuthSubject *subject, - const char *action_id, - gboolean allow_user_interaction, - GCancellable *cancellable, - GAsyncReadyCallback callback, - gpointer user_data); -gboolean nm_auth_manager_polkit_authority_check_authorization_finish (NMAuthManager *self, - GAsyncResult *res, - gboolean *out_is_authorized, - gboolean *out_is_challenge, - GError **error); +typedef struct _NMAuthManagerCallId NMAuthManagerCallId; -#endif +typedef void (*NMAuthManagerCheckAuthorizationCallback) (NMAuthManager *self, + NMAuthManagerCallId *call_id, + gboolean is_authorized, + gboolean is_challenge, + GError *error, + gpointer user_data); + +NMAuthManagerCallId *nm_auth_manager_check_authorization (NMAuthManager *self, + NMAuthSubject *subject, + const char *action_id, + gboolean allow_user_interaction, + NMAuthManagerCheckAuthorizationCallback callback, + gpointer user_data); + +void nm_auth_manager_check_authorization_cancel (NMAuthManagerCallId *call_id); #endif /* NM_AUTH_MANAGER_H */ diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 3c5440eb2b..ffb8f39d78 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -52,7 +52,7 @@ struct NMAuthChain { typedef struct { CList auth_call_lst; NMAuthChain *chain; - GCancellable *cancellable; + NMAuthManagerCallId *call_id; char *permission; guint call_idle_id; } AuthCall; @@ -72,8 +72,12 @@ _ASSERT_call (AuthCall *call) static void auth_call_free (AuthCall *call) { +#if WITH_POLKIT + if (call->call_id) + nm_auth_manager_check_authorization_cancel (call->call_id); +#endif + nm_clear_g_source (&call->call_idle_id); - nm_clear_g_cancellable (&call->cancellable); c_list_unlink_stale (&call->auth_call_lst); g_free (call->permission); g_slice_free (AuthCall, call); @@ -253,26 +257,24 @@ auth_call_complete_idle_cb (gpointer user_data) #if WITH_POLKIT static void -pk_call_cb (GObject *object, GAsyncResult *result, gpointer user_data) +pk_call_cb (NMAuthManager *auth_manager, + NMAuthManagerCallId *call_id, + gboolean is_authorized, + gboolean is_challenge, + GError *error, + gpointer user_data) { AuthCall *call; - gs_free_error GError *error = NULL; - gboolean is_authorized = FALSE, is_challenge = FALSE; - guint call_result = NM_AUTH_CALL_RESULT_UNKNOWN; + NMAuthCallResult call_result = NM_AUTH_CALL_RESULT_UNKNOWN; - nm_auth_manager_polkit_authority_check_authorization_finish (NM_AUTH_MANAGER (object), - result, - &is_authorized, - &is_challenge, - &error); - if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { - nm_log_dbg (LOGD_CORE, "callback already cancelled"); + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; - } call = user_data; - g_clear_object (&call->cancellable); + nm_assert (call->call_id == call_id); + + call->call_id = NULL; if (error) { /* Don't ruin the chain. Just leave the result unknown. */ @@ -323,14 +325,12 @@ nm_auth_chain_add_call (NMAuthChain *self, } else { /* Non-root always gets authenticated when using polkit */ #if WITH_POLKIT - call->cancellable = g_cancellable_new (); - nm_auth_manager_polkit_authority_check_authorization (auth_manager, - self->subject, - permission, - allow_interaction, - call->cancellable, - pk_call_cb, - call); + call->call_id = nm_auth_manager_check_authorization (auth_manager, + self->subject, + permission, + allow_interaction, + pk_call_cb, + call); #else if (!call->chain->error) { call->chain->error = g_error_new_literal (NM_MANAGER_ERROR, From 41abf9f8e81423eff0ef888d17a5454d0b5750bf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 18:13:28 +0200 Subject: [PATCH 32/40] auth-manager: always compile D-Bus calls to polkit Supporting PolicyKit required no additional library, just extra code to handle the D-Bus calls. For that, there was a compile time option to even stip out that code. Note, that you could (and still can) configure the system not to use policy-kit. The point was to reduce the binary size in case you don't need it. Remove this. I guess, we we aim for such aggressive optimization of the binary size, we should instead make all device types disablable at configuration time. We don't do that either and other low hanging fruits, because it's better to always enable features, unless they require external dependencies. Also, the next commit will make more use of NMAuthManager. So, having it disabled at compile time, makes even less sense. --- config.h.meson | 3 --- configure.ac | 26 ++++++++------------------ meson.build | 1 - src/nm-auth-manager.c | 22 ---------------------- src/nm-auth-utils.c | 13 ------------- 5 files changed, 8 insertions(+), 57 deletions(-) diff --git a/config.h.meson b/config.h.meson index 12b35a66b9..06190aae11 100644 --- a/config.h.meson +++ b/config.h.meson @@ -214,9 +214,6 @@ /* Define if you have oFono support (experimental) */ #mesondefine WITH_OFONO -/* whether to compile polkit support */ -#mesondefine WITH_POLKIT - /* Define if you have polkit agent */ #mesondefine WITH_POLKIT_AGENT diff --git a/configure.ac b/configure.ac index a0600e1ca7..c703890e1c 100644 --- a/configure.ac +++ b/configure.ac @@ -629,26 +629,20 @@ AM_CONDITIONAL(WITH_JSON_VALIDATION, test "${enable_json_validation}" != "no") # we usually compile with polkit support. --enable-polkit=yes|no only sets the # default configuration for main.auth-polkit. User can always enable/disable polkit -# autorization via config. Only when specifying --enable-polkit=disabled, we do -# not compile support. In this case, the user cannot enable polkit authorization via -# configuration. +# autorization via config. AC_ARG_ENABLE(polkit, - AS_HELP_STRING([--enable-polkit=yes|no|disabled], - [set default value for auth-polkit configuration option. This value can be overwritten by NM configuration. 'disabled' compiles NM without any support]), + AS_HELP_STRING([--enable-polkit=yes|no], + [set default value for auth-polkit configuration option. This value can be overwritten by NM configuration. 'disabled' is an alias for 'no']), [enable_polkit=${enableval}], [enable_polkit=yes]) if (test "${enable_polkit}" != "no" -a "${enable_polkit}" != "disabled"); then - enable_polkit=yes + enable_polkit=true AC_DEFINE(NM_CONFIG_DEFAULT_MAIN_AUTH_POLKIT, "true", [The default value of the auth-polkit configuration option]) AC_SUBST(NM_CONFIG_DEFAULT_MAIN_AUTH_POLKIT_TEXT, true) else + enable_polkit=false AC_DEFINE(NM_CONFIG_DEFAULT_MAIN_AUTH_POLKIT, "false", [The default value of the auth-polkit configuration option]) AC_SUBST(NM_CONFIG_DEFAULT_MAIN_AUTH_POLKIT_TEXT, false) fi -if (test "${enable_polkit}" != "disabled"); then - AC_DEFINE(WITH_POLKIT, 1, [whether to compile polkit support]) -else - AC_DEFINE(WITH_POLKIT, 0, [whether to compile polkit support]) -fi PKG_CHECK_MODULES(POLKIT, [polkit-agent-1 >= 0.97], [have_pk_agent=yes],[have_pk_agent=no]) AC_ARG_ENABLE(polkit-agent, @@ -1341,14 +1335,10 @@ echo echo "Platform:" echo " session tracking: $session_tracking" echo " suspend/resume: $with_suspend_resume" -if test "${enable_polkit}" = "yes"; then - if test "${enable_modify_system}" = "yes"; then - echo " policykit: yes (permissive modify.system) (default: main.auth-polkit=${enable_polkit})" - else - echo " policykit: yes (restrictive modify.system) (default: main.auth-polkit=${enable_polkit})" - fi +if test "${enable_modify_system}" = "yes"; then + echo " policykit: main.auth-polkit=${enable_polkit} (permissive modify.system)" else - echo " policykit: no" + echo " policykit: main.auth-polkit=${enable_polkit} (restrictive modify.system)" fi echo " polkit agent: ${enable_polkit_agent}" echo " selinux: $have_selinux" diff --git a/meson.build b/meson.build index 5f29c16a69..17ddbc4a57 100644 --- a/meson.build +++ b/meson.build @@ -448,7 +448,6 @@ endif config_default_main_auth_polkit = (polkit == 'yes').to_string() config_h.set_quoted('NM_CONFIG_DEFAULT_MAIN_AUTH_POLKIT', config_default_main_auth_polkit) -config_h.set10('WITH_POLKIT', enable_polkit) enable_modify_system = get_option('modify_system') diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index 784a8f23f1..ee63df6f56 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -48,13 +48,11 @@ enum { static guint signals[LAST_SIGNAL] = {0}; typedef struct { -#if WITH_POLKIT CList calls_lst_head; GDBusProxy *proxy; GCancellable *new_proxy_cancellable; GCancellable *cancel_cancellable; guint64 call_numid_counter; -#endif bool polkit_enabled:1; bool disposing:1; bool shutting_down:1; @@ -120,8 +118,6 @@ nm_auth_manager_get_polkit_enabled (NMAuthManager *self) /*****************************************************************************/ -#if WITH_POLKIT - typedef enum { POLKIT_CHECK_AUTHORIZATION_FLAGS_NONE = 0, POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION = (1<<0), @@ -514,7 +510,6 @@ _dbus_new_proxy_cb (GObject *source_object, _emit_changed_signal (self); } -#endif /*****************************************************************************/ @@ -529,7 +524,6 @@ nm_auth_manager_get () void nm_auth_manager_force_shutdown (NMAuthManager *self) { -#if WITH_POLKIT NMAuthManagerPrivate *priv; g_return_if_fail (NM_IS_AUTH_MANAGER (self)); @@ -559,9 +553,6 @@ nm_auth_manager_force_shutdown (NMAuthManager *self) priv->shutting_down = TRUE; nm_clear_g_cancellable (&priv->cancel_cancellable); -#else - g_return_if_fail (NM_IS_AUTH_MANAGER (self)); -#endif } /*****************************************************************************/ @@ -587,11 +578,9 @@ set_property (GObject *object, guint prop_id, const GValue *value, GParamSpec *p static void nm_auth_manager_init (NMAuthManager *self) { -#if WITH_POLKIT NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); c_list_init (&priv->calls_lst_head); -#endif } static void @@ -602,7 +591,6 @@ constructed (GObject *object) G_OBJECT_CLASS (nm_auth_manager_parent_class)->constructed (object); -#if WITH_POLKIT _LOGD ("create auth-manager: polkit %s", priv->polkit_enabled ? "enabled" : "disabled"); if (priv->polkit_enabled) { @@ -617,12 +605,6 @@ constructed (GObject *object) _dbus_new_proxy_cb, self); } -#else - if (priv->polkit_enabled) - _LOGW ("create auth-manager: polkit disabled at compile time. All authentication requests will fail"); - else - _LOGD ("create auth-manager: polkit disabled at compile time"); -#endif } NMAuthManager * @@ -649,14 +631,11 @@ static void dispose (GObject *object) { NMAuthManager* self = NM_AUTH_MANAGER (object); -#if WITH_POLKIT NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); gs_free_error GError *error_disposing = NULL; -#endif _LOGD ("dispose"); -#if WITH_POLKIT nm_assert (!c_list_is_empty (&priv->calls_lst_head)); priv->disposing = TRUE; @@ -668,7 +647,6 @@ dispose (GObject *object) g_signal_handlers_disconnect_by_data (priv->proxy, self); g_clear_object (&priv->proxy); } -#endif G_OBJECT_CLASS (nm_auth_manager_parent_class)->dispose (object); } diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index ffb8f39d78..d641621bf2 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -72,10 +72,8 @@ _ASSERT_call (AuthCall *call) static void auth_call_free (AuthCall *call) { -#if WITH_POLKIT if (call->call_id) nm_auth_manager_check_authorization_cancel (call->call_id); -#endif nm_clear_g_source (&call->call_idle_id); c_list_unlink_stale (&call->auth_call_lst); @@ -255,7 +253,6 @@ auth_call_complete_idle_cb (gpointer user_data) return G_SOURCE_REMOVE; } -#if WITH_POLKIT static void pk_call_cb (NMAuthManager *auth_manager, NMAuthManagerCallId *call_id, @@ -295,7 +292,6 @@ pk_call_cb (NMAuthManager *auth_manager, auth_call_complete (call); } -#endif void nm_auth_chain_add_call (NMAuthChain *self, @@ -324,21 +320,12 @@ nm_auth_chain_add_call (NMAuthChain *self, call->call_idle_id = g_idle_add (auth_call_complete_idle_cb, call); } else { /* Non-root always gets authenticated when using polkit */ -#if WITH_POLKIT call->call_id = nm_auth_manager_check_authorization (auth_manager, self->subject, permission, allow_interaction, pk_call_cb, call); -#else - if (!call->chain->error) { - call->chain->error = g_error_new_literal (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_FAILED, - "Polkit support is disabled at compile time"); - } - call->call_idle_id = g_idle_add (auth_call_complete_idle_cb, call); -#endif } } From 798b2a7527bddadcec37b48183da313fbc961e45 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 16:04:46 +0200 Subject: [PATCH 33/40] auth-manager: let NMAuthChain always call to NMAuthManager for dummy requests NMAuthChain's nm_auth_chain_add_call() used to add special handling for the NMAuthSubject. This handling really belongs to NMAuthManager for two reasons: - NMAuthManager already goes through the effort of scheduling an idle handler to handle the case where no GDBusProxy is present. It can just as well handle the special cases where polkit-auth is disabled or when we have internal requests. - by NMAuthChain doing special handling, it makes it more complicated to call nm_auth_manager_check_authorization() directly. Previously, the NMAuthChain had additional logic, which means you either were forced to create an NMAuthChain, or you had to reimplement special handling like nm_auth_chain_add_call(). --- src/nm-auth-manager.c | 54 ++++++++++++++++++++++++++++++++++--------- src/nm-auth-utils.c | 37 +++++------------------------ 2 files changed, 49 insertions(+), 42 deletions(-) diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index ee63df6f56..6a8214c824 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -123,6 +123,11 @@ typedef enum { POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION = (1<<0), } PolkitCheckAuthorizationFlags; +typedef enum { + IDLE_REASON_AUTHORIZED, + IDLE_REASON_NO_DBUS, +} IdleReason; + struct _NMAuthManagerCallId { CList calls_lst; NMAuthManager *self; @@ -132,6 +137,7 @@ struct _NMAuthManagerCallId { gpointer user_data; guint64 call_numid; guint idle_id; + IdleReason idle_reason:8; }; #define cancellation_id_to_str_a(call_numid) \ @@ -289,16 +295,28 @@ _call_check_authorize (NMAuthManagerCallId *call_id) } static gboolean -_call_fail_on_idle (gpointer user_data) +_call_on_idle (gpointer user_data) { NMAuthManagerCallId *call_id = user_data; gs_free_error GError *error = NULL; + gboolean is_authorized = FALSE; + gboolean is_challenge = FALSE; + const char *error_msg = NULL; call_id->idle_id = 0; - g_set_error_literal (&error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "failure creating GDBusProxy for authorization request"); - _LOG2T (call_id, "completed: failed due to no D-Bus proxy"); - _call_id_invoke_callback (call_id, FALSE, FALSE, error); + if (call_id->idle_reason == IDLE_REASON_AUTHORIZED) { + is_authorized = TRUE; + _LOG2T (call_id, "completed: authorized=%d, challenge=%d (simulated)", + is_authorized, is_challenge); + } else { + nm_assert (call_id->idle_reason == IDLE_REASON_NO_DBUS); + error_msg = "failure creating GDBusProxy for authorization request"; + _LOG2T (call_id, "completed: failed due to no D-Bus proxy"); + } + + if (error_msg) + g_set_error_literal (&error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, error_msg); + _call_id_invoke_callback (call_id, is_authorized, is_challenge, error); return G_SOURCE_REMOVE; } @@ -332,13 +350,14 @@ nm_auth_manager_check_authorization (NMAuthManager *self, NMAuthManagerCallId *call_id; g_return_val_if_fail (NM_IS_AUTH_MANAGER (self), NULL); - g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL); - g_return_val_if_fail (nm_auth_subject_is_unix_process (subject), NULL); + g_return_val_if_fail (NM_IN_SET (nm_auth_subject_get_subject_type (subject), + NM_AUTH_SUBJECT_TYPE_INTERNAL, + NM_AUTH_SUBJECT_TYPE_UNIX_PROCESS), + NULL); g_return_val_if_fail (action_id, NULL); priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - g_return_val_if_fail (priv->polkit_enabled, NULL); g_return_val_if_fail (!priv->disposing, NULL); g_return_val_if_fail (!priv->shutting_down, NULL); @@ -353,10 +372,23 @@ nm_auth_manager_check_authorization (NMAuthManager *self, call_id->call_numid = ++priv->call_numid_counter; c_list_link_tail (&priv->calls_lst_head, &call_id->calls_lst); - if ( !priv->proxy - && !priv->new_proxy_cancellable) { + if (!priv->polkit_enabled) { + _LOG2T (call_id, "CheckAuthorization(%s), subject=%s (succeeding due to polkit authorization disabled)", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); + call_id->idle_reason = IDLE_REASON_AUTHORIZED; + call_id->idle_id = g_idle_add (_call_on_idle, call_id); + } else if (nm_auth_subject_is_internal (subject)) { + _LOG2T (call_id, "CheckAuthorization(%s), subject=%s (succeeding for internal request)", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); + call_id->idle_reason = IDLE_REASON_AUTHORIZED; + call_id->idle_id = g_idle_add (_call_on_idle, call_id); + } else if (nm_auth_subject_get_unix_process_uid (subject) == 0) { + _LOG2T (call_id, "CheckAuthorization(%s), subject=%s (succeeding for root)", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); + call_id->idle_reason = IDLE_REASON_AUTHORIZED; + call_id->idle_id = g_idle_add (_call_on_idle, call_id); + } else if ( !priv->proxy + && !priv->new_proxy_cancellable) { _LOG2T (call_id, "CheckAuthorization(%s), subject=%s (failing due to invalid DBUS proxy)", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); - call_id->idle_id = g_idle_add (_call_fail_on_idle, call_id); + call_id->idle_reason = IDLE_REASON_NO_DBUS; + call_id->idle_id = g_idle_add (_call_on_idle, call_id); } else { subject_value = nm_auth_subject_unix_process_to_polkit_gvariant (subject); nm_assert (g_variant_is_floating (subject_value)); diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index d641621bf2..83206352cd 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -54,7 +54,6 @@ typedef struct { NMAuthChain *chain; NMAuthManagerCallId *call_id; char *permission; - guint call_idle_id; } AuthCall; /*****************************************************************************/ @@ -74,8 +73,6 @@ auth_call_free (AuthCall *call) { if (call->call_id) nm_auth_manager_check_authorization_cancel (call->call_id); - - nm_clear_g_source (&call->call_idle_id); c_list_unlink_stale (&call->auth_call_lst); g_free (call->permission); g_slice_free (AuthCall, call); @@ -241,18 +238,6 @@ auth_call_complete (AuthCall *call) } } -static gboolean -auth_call_complete_idle_cb (gpointer user_data) -{ - AuthCall *call = user_data; - - _ASSERT_call (call); - - call->call_idle_id = 0; - auth_call_complete (call); - return G_SOURCE_REMOVE; -} - static void pk_call_cb (NMAuthManager *auth_manager, NMAuthManagerCallId *call_id, @@ -311,22 +296,12 @@ nm_auth_chain_add_call (NMAuthChain *self, call->chain = self; call->permission = g_strdup (permission); c_list_link_tail (&self->auth_call_lst_head, &call->auth_call_lst); - - if ( nm_auth_subject_is_internal (self->subject) - || nm_auth_subject_get_unix_process_uid (self->subject) == 0 - || !nm_auth_manager_get_polkit_enabled (auth_manager)) { - /* Root user or non-polkit always gets the permission */ - nm_auth_chain_set_data (self, permission, GUINT_TO_POINTER (NM_AUTH_CALL_RESULT_YES), NULL); - call->call_idle_id = g_idle_add (auth_call_complete_idle_cb, call); - } else { - /* Non-root always gets authenticated when using polkit */ - call->call_id = nm_auth_manager_check_authorization (auth_manager, - self->subject, - permission, - allow_interaction, - pk_call_cb, - call); - } + call->call_id = nm_auth_manager_check_authorization (auth_manager, + self->subject, + permission, + allow_interaction, + pk_call_cb, + call); } /*****************************************************************************/ From f0bddb44e03aed9d9ca8be0bf63d46dd7ebda15c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 17:03:58 +0200 Subject: [PATCH 34/40] auth-manager: add helper function nm_auth_call_result_eval() This makes NMAuthCallResult not only usable from within a NMAuthChain. It makes sense to just call nm-auth-manager directly, but then we need a way to convert the more detailed result into an NMAuthCallResult value. --- src/nm-auth-manager.h | 25 +++++++++++++++++++++++++ src/nm-auth-utils.c | 13 +++---------- src/nm-auth-utils.h | 9 ++------- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/nm-auth-manager.h b/src/nm-auth-manager.h index b84debcd6b..fe7ee787dc 100644 --- a/src/nm-auth-manager.h +++ b/src/nm-auth-manager.h @@ -23,6 +23,31 @@ #include "nm-auth-subject.h" +/*****************************************************************************/ + +typedef enum { + NM_AUTH_CALL_RESULT_UNKNOWN, + NM_AUTH_CALL_RESULT_YES, + NM_AUTH_CALL_RESULT_AUTH, + NM_AUTH_CALL_RESULT_NO, +} NMAuthCallResult; + +static inline NMAuthCallResult +nm_auth_call_result_eval (gboolean is_authorized, + gboolean is_challenge, + GError *error) +{ + if (error) + return NM_AUTH_CALL_RESULT_UNKNOWN; + if (is_authorized) + return NM_AUTH_CALL_RESULT_YES; + if (is_challenge) + return NM_AUTH_CALL_RESULT_AUTH; + return NM_AUTH_CALL_RESULT_NO; +} + +/*****************************************************************************/ + #define NM_TYPE_AUTH_MANAGER (nm_auth_manager_get_type ()) #define NM_AUTH_MANAGER(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), NM_TYPE_AUTH_MANAGER, NMAuthManager)) #define NM_AUTH_MANAGER_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), NM_TYPE_AUTH_MANAGER, NMAuthManagerClass)) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 83206352cd..c04dd987a0 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -247,7 +247,7 @@ pk_call_cb (NMAuthManager *auth_manager, gpointer user_data) { AuthCall *call; - NMAuthCallResult call_result = NM_AUTH_CALL_RESULT_UNKNOWN; + NMAuthCallResult call_result; if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; @@ -262,17 +262,10 @@ pk_call_cb (NMAuthManager *auth_manager, /* Don't ruin the chain. Just leave the result unknown. */ nm_log_warn (LOGD_CORE, "error requesting auth for %s: %s", call->permission, error->message); - } else { - if (is_authorized) { - /* Caller has the permission */ - call_result = NM_AUTH_CALL_RESULT_YES; - } else if (is_challenge) { - /* Caller could authenticate to get the permission */ - call_result = NM_AUTH_CALL_RESULT_AUTH; - } else - call_result = NM_AUTH_CALL_RESULT_NO; } + call_result = nm_auth_call_result_eval (is_authorized, is_challenge, error); + nm_auth_chain_set_data (call->chain, call->permission, GUINT_TO_POINTER (call_result), NULL); auth_call_complete (call); diff --git a/src/nm-auth-utils.h b/src/nm-auth-utils.h index 808da86a51..a641c49dfe 100644 --- a/src/nm-auth-utils.h +++ b/src/nm-auth-utils.h @@ -23,14 +23,9 @@ #include "nm-connection.h" -typedef struct NMAuthChain NMAuthChain; +#include "nm-auth-manager.h" -typedef enum { - NM_AUTH_CALL_RESULT_UNKNOWN, - NM_AUTH_CALL_RESULT_YES, - NM_AUTH_CALL_RESULT_AUTH, - NM_AUTH_CALL_RESULT_NO, -} NMAuthCallResult; +typedef struct NMAuthChain NMAuthChain; typedef void (*NMAuthChainResultFunc) (NMAuthChain *chain, GError *error, From 6ab0939e340d43641093f2e825e3d20b1815b3b2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 17:35:20 +0200 Subject: [PATCH 35/40] settings: cancel pending authorization requests if connection gets removed Otherwise, the autorization request might succeed and we would still do something with the connection that is already removed. https://bugzilla.redhat.com/show_bug.cgi?id=1565030 --- src/settings/nm-settings-connection.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index e72a623e7b..43391cc3f5 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -2276,11 +2276,19 @@ void nm_settings_connection_signal_remove (NMSettingsConnection *self) { NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE (self); + GSList *pending_auth; if (priv->removed) return; priv->removed = TRUE; + while ((pending_auth = priv->pending_auths)) { + NMAuthChain *chain = pending_auth->data; + + priv->pending_auths = g_slist_delete_link (priv->pending_auths, pending_auth); + nm_auth_chain_destroy (chain); + } + nm_dbus_object_emit_signal (NM_DBUS_OBJECT (self), &interface_info_settings_connection, &signal_info_removed, From 9385c7a7cfe9ac5dcc60f638d2ab9d7768e832c4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 12:48:34 +0200 Subject: [PATCH 36/40] settings: rework scheduling of authorization requests in settings-connection Get rid of the NMAuthChain layer. I think NMAuthChain only makes sense if we schedule multiple requests together for the same topic. But NMSettingsConnection never does that: each D-Bus request corresponds to only one polkit authorization request. So, we can just call NMAuthManager directly. --- src/settings/nm-settings-connection.c | 139 ++++++++++++++------------ 1 file changed, 74 insertions(+), 65 deletions(-) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 43391cc3f5..9e87a35ba3 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -32,6 +32,7 @@ #include "nm-config-data.h" #include "nm-dbus-interface.h" #include "nm-session-monitor.h" +#include "nm-auth-manager.h" #include "nm-auth-utils.h" #include "nm-auth-subject.h" #include "nm-agent-manager.h" @@ -80,7 +81,8 @@ typedef struct _NMSettingsConnectionPrivate { NMSettingsAutoconnectBlockedReason autoconnect_blocked_reason:4; - GSList *pending_auths; /* List of pending authentication requests */ + /* List of pending authentication requests */ + CList auth_lst_head; CList call_ids_lst_head; /* in-progress secrets requests */ @@ -1361,7 +1363,7 @@ nm_settings_connection_cancel_secrets (NMSettingsConnection *self, _get_secrets_cancel (self, call_id, FALSE); } -/**** User authorization **************************************/ +/*****************************************************************************/ typedef void (*AuthCallback) (NMSettingsConnection *self, GDBusMethodInvocation *context, @@ -1369,46 +1371,61 @@ typedef void (*AuthCallback) (NMSettingsConnection *self, GError *error, gpointer data); -static void -pk_auth_cb (NMAuthChain *chain, - GError *chain_error, - GDBusMethodInvocation *context, - gpointer user_data) -{ - NMSettingsConnection *self = NM_SETTINGS_CONNECTION (user_data); - NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE (self); - GError *error = NULL; - NMAuthCallResult result; - const char *perm; +typedef struct { + CList auth_lst; + NMAuthManagerCallId *call_id; + NMSettingsConnection *self; AuthCallback callback; gpointer callback_data; + GDBusMethodInvocation *invocation; NMAuthSubject *subject; +} AuthData; - priv->pending_auths = g_slist_remove (priv->pending_auths, chain); +static void +pk_auth_cb (NMAuthManager *auth_manager, + NMAuthManagerCallId *auth_call_id, + gboolean is_authorized, + gboolean is_challenge, + GError *auth_error, + gpointer user_data) +{ + AuthData *auth_data = user_data; + NMSettingsConnection *self; + gs_free_error GError *error = NULL; - perm = nm_auth_chain_get_data (chain, "perm"); - g_assert (perm); - result = nm_auth_chain_get_result (chain, perm); + nm_assert (auth_data); + nm_assert (NM_IS_SETTINGS_CONNECTION (auth_data->self)); - /* If our NMSettingsConnection is already gone, do nothing */ - if (chain_error) { + self = auth_data->self; + + auth_data->call_id = NULL; + + c_list_unlink (&auth_data->auth_lst); + + if (g_error_matches (auth_error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { + error = g_error_new (NM_SETTINGS_ERROR, + NM_SETTINGS_ERROR_FAILED, + "Error checking authorization: connection was deleted"); + } else if (auth_error) { error = g_error_new (NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, "Error checking authorization: %s", - chain_error->message ? chain_error->message : "(unknown)"); - } else if (result != NM_AUTH_CALL_RESULT_YES) { + auth_error->message); + } else if (nm_auth_call_result_eval (is_authorized, is_challenge, auth_error) != NM_AUTH_CALL_RESULT_YES) { error = g_error_new_literal (NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_PERMISSION_DENIED, - "Insufficient privileges."); + "Insufficient privileges"); } - callback = nm_auth_chain_get_data (chain, "callback"); - callback_data = nm_auth_chain_get_data (chain, "callback-data"); - subject = nm_auth_chain_get_data (chain, "subject"); - callback (self, context, subject, error, callback_data); + auth_data->callback (self, + auth_data->invocation, + auth_data->subject, + error, + auth_data->callback_data); - g_clear_error (&error); - nm_auth_chain_destroy (chain); + g_object_unref (auth_data->invocation); + g_object_unref (auth_data->subject); + g_slice_free (AuthData, auth_data); } /** @@ -1436,59 +1453,57 @@ _new_auth_subject (GDBusMethodInvocation *context, GError **error) return subject; } +/* may either invoke callback synchronously or asynchronously. */ static void auth_start (NMSettingsConnection *self, - GDBusMethodInvocation *context, + GDBusMethodInvocation *invocation, NMAuthSubject *subject, const char *check_permission, AuthCallback callback, gpointer callback_data) { NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE (self); - NMAuthChain *chain; - GError *error = NULL; + AuthData *auth_data; char *error_desc = NULL; - g_return_if_fail (context != NULL); - g_return_if_fail (NM_IS_AUTH_SUBJECT (subject)); + nm_assert (nm_dbus_object_is_exported (NM_DBUS_OBJECT (self))); + nm_assert (G_IS_DBUS_METHOD_INVOCATION (invocation)); + nm_assert (NM_IS_AUTH_SUBJECT (subject)); /* Ensure the caller can view this connection */ if (!nm_auth_is_subject_in_acl (NM_CONNECTION (self), subject, &error_desc)) { + gs_free_error GError *error = NULL; + error = g_error_new_literal (NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_PERMISSION_DENIED, error_desc); g_free (error_desc); - callback (self, context, subject, error, callback_data); - g_clear_error (&error); + callback (self, invocation, subject, error, callback_data); return; } if (!check_permission) { /* Don't need polkit auth, automatic success */ - callback (self, context, subject, NULL, callback_data); + callback (self, invocation, subject, NULL, callback_data); return; } - chain = nm_auth_chain_new_subject (subject, context, pk_auth_cb, self); - if (!chain) { - g_set_error_literal (&error, - NM_SETTINGS_ERROR, - NM_SETTINGS_ERROR_PERMISSION_DENIED, - "Unable to authenticate the request."); - callback (self, context, subject, error, callback_data); - g_clear_error (&error); - return; - } - - priv->pending_auths = g_slist_append (priv->pending_auths, chain); - nm_auth_chain_set_data (chain, "perm", (gpointer) check_permission, NULL); - nm_auth_chain_set_data (chain, "callback", callback, NULL); - nm_auth_chain_set_data (chain, "callback-data", callback_data, NULL); - nm_auth_chain_set_data (chain, "subject", g_object_ref (subject), g_object_unref); - nm_auth_chain_add_call (chain, check_permission, TRUE); + auth_data = g_slice_new (AuthData); + auth_data->self = self; + auth_data->callback = callback; + auth_data->callback_data = callback_data; + auth_data->invocation = g_object_ref (invocation); + auth_data->subject = g_object_ref (subject); + c_list_link_tail (&priv->auth_lst_head, &auth_data->auth_lst); + auth_data->call_id = nm_auth_manager_check_authorization (nm_auth_manager_get (), + subject, + check_permission, + TRUE, + pk_auth_cb, + auth_data); } /**** DBus method handlers ************************************/ @@ -2067,7 +2082,7 @@ get_modify_permission_basic (NMSettingsConnection *self) * request affects more than just the caller, require 'modify.system'. */ s_con = nm_connection_get_setting_connection (NM_CONNECTION (self)); - g_assert (s_con); + nm_assert (s_con); if (nm_setting_connection_get_num_permissions (s_con) == 1) return NM_AUTH_PERMISSION_SETTINGS_MODIFY_OWN; @@ -2276,18 +2291,14 @@ void nm_settings_connection_signal_remove (NMSettingsConnection *self) { NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE (self); - GSList *pending_auth; + AuthData *auth_data; if (priv->removed) return; priv->removed = TRUE; - while ((pending_auth = priv->pending_auths)) { - NMAuthChain *chain = pending_auth->data; - - priv->pending_auths = g_slist_delete_link (priv->pending_auths, pending_auth); - nm_auth_chain_destroy (chain); - } + while ((auth_data = c_list_first_entry (&priv->auth_lst_head, AuthData, auth_lst))) + nm_auth_manager_check_authorization_cancel (auth_data->call_id); nm_dbus_object_emit_signal (NM_DBUS_OBJECT (self), &interface_info_settings_connection, @@ -2977,6 +2988,7 @@ nm_settings_connection_init (NMSettingsConnection *self) priv->ready = TRUE; c_list_init (&priv->call_ids_lst_head); + c_list_init (&priv->auth_lst_head); priv->session_monitor = g_object_ref (nm_session_monitor_get ()); priv->session_changed_id = g_signal_connect (priv->session_monitor, @@ -3013,6 +3025,7 @@ dispose (GObject *object) _LOGD ("disposing"); nm_assert (c_list_is_empty (&self->_connections_lst)); + nm_assert (c_list_is_empty (&priv->auth_lst_head)); /* Cancel in-progress secrets requests */ if (priv->agent_mgr) { @@ -3031,10 +3044,6 @@ dispose (GObject *object) g_clear_object (&priv->system_secrets); g_clear_object (&priv->agent_secrets); - /* Cancel PolicyKit requests */ - g_slist_free_full (priv->pending_auths, (GDestroyNotify) nm_auth_chain_destroy); - priv->pending_auths = NULL; - g_clear_pointer (&priv->seen_bssids, g_hash_table_destroy); set_visible (self, FALSE); From 87227038924ecce336f8605eaed034b5b22d1b7c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Apr 2018 18:48:05 +0200 Subject: [PATCH 37/40] auth-chain: drop logging in NMAuthChain when request fails For one, we already do level logging inside NMAuthManager. So, at trace level we have everything. If a request fails, it's not up to NMAuthChain to log a warning. --- src/nm-auth-utils.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index c04dd987a0..4d268d0025 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -258,12 +258,6 @@ pk_call_cb (NMAuthManager *auth_manager, call->call_id = NULL; - if (error) { - /* Don't ruin the chain. Just leave the result unknown. */ - nm_log_warn (LOGD_CORE, "error requesting auth for %s: %s", - call->permission, error->message); - } - call_result = nm_auth_call_result_eval (is_authorized, is_challenge, error); nm_auth_chain_set_data (call->chain, call->permission, GUINT_TO_POINTER (call_result), NULL); From c05088a09b847c750f4ad8144f26a6273090ec4d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 Apr 2018 08:46:01 +0200 Subject: [PATCH 38/40] core: don't use NMAuthChain in NMActiveConnection but directly schedule events More of a proof of concept, how convenient (or not) it is to drop NMAuthChain and use NMAuthManager's API directly. I think it's reasonably nice. As before, when asking for both general network-control permissions and wifi-shared-permissions, we will not fail with wifi-shared-permissions, as long as network-control check is still pending. The effect is, that the error response preferably complains about no permissions to network-control (in case both permissions are missing). One change in behavior is, if network-control authorization check fails before wifi-shared-permissions, we declare the result and cancel the pending wifi-shared-permissions. Previously, we would have waited for both results. The change in behavior is not merely that we declare the result earlier, but also that NMAuthManager will actively send a "CancelCheckAuthorization" D-Bus call to cancel the still pending wifi-shared-permissions check. --- src/nm-active-connection.c | 169 +++++++++++++++++++++++-------------- 1 file changed, 105 insertions(+), 64 deletions(-) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index a02deb3b5b..4f6033893b 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -28,10 +28,13 @@ #include "settings/nm-settings-connection.h" #include "nm-simple-connection.h" #include "nm-auth-utils.h" +#include "nm-auth-manager.h" #include "nm-auth-subject.h" #include "NetworkManagerUtils.h" #include "nm-core-internal.h" +#define AUTH_CALL_ID_SHARED_WIFI_PERMISSION_FAILED ((NMAuthManagerCallId *) GINT_TO_POINTER (1)) + typedef struct _NMActiveConnectionPrivate { NMDBusTrackObjPath settings_connection; NMConnection *applied_connection; @@ -59,11 +62,15 @@ typedef struct _NMActiveConnectionPrivate { NMActiveConnection *parent; - NMAuthChain *chain; - const char *wifi_shared_permission; - NMActiveConnectionAuthResultFunc result_func; - gpointer user_data1; - gpointer user_data2; + struct { + NMAuthManagerCallId *call_id_network_control; + NMAuthManagerCallId *call_id_wifi_shared_permission; + + NMActiveConnectionAuthResultFunc result_func; + gpointer user_data1; + gpointer user_data2; + } auth; + } NMActiveConnectionPrivate; NM_GOBJECT_PROPERTIES_DEFINE (NMActiveConnection, @@ -981,62 +988,93 @@ nm_active_connection_set_parent (NMActiveConnection *self, NMActiveConnection *p /*****************************************************************************/ static void -auth_done (NMAuthChain *chain, +auth_cancel (NMActiveConnection *self) +{ + NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); + + if (priv->auth.call_id_network_control) + nm_auth_manager_check_authorization_cancel (priv->auth.call_id_network_control); + if (priv->auth.call_id_wifi_shared_permission) { + if (priv->auth.call_id_wifi_shared_permission == AUTH_CALL_ID_SHARED_WIFI_PERMISSION_FAILED) + priv->auth.call_id_wifi_shared_permission = NULL; + else + nm_auth_manager_check_authorization_cancel (priv->auth.call_id_wifi_shared_permission); + } + priv->auth.result_func = NULL; + priv->auth.user_data1 = NULL; + priv->auth.user_data2 = NULL; +} + +static void +auth_complete (NMActiveConnection *self, gboolean result, const char *message) +{ + _nm_unused gs_unref_object NMActiveConnection *self_keep_alive = g_object_ref (self); + NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); + + priv->auth.result_func (self, + result, + message, + priv->auth.user_data1, + priv->auth.user_data2); + auth_cancel (self); +} + +static void +auth_done (NMAuthManager *auth_mgr, + NMAuthManagerCallId *auth_call_id, + gboolean is_authorized, + gboolean is_challenge, GError *error, - GDBusMethodInvocation *unused, gpointer user_data) + { NMActiveConnection *self = NM_ACTIVE_CONNECTION (user_data); NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); NMAuthCallResult result; - g_assert (priv->chain == chain); - g_assert (priv->result_func != NULL); + nm_assert (auth_call_id); + nm_assert (priv->auth.result_func); - /* Must stay alive over the callback */ - g_object_ref (self); - - if (error) { - priv->result_func (self, FALSE, error->message, priv->user_data1, priv->user_data2); - goto done; - } - - /* Caller has had a chance to obtain authorization, so we only need to - * check for 'yes' here. - */ - result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_NETWORK_CONTROL); - if (result != NM_AUTH_CALL_RESULT_YES) { - priv->result_func (self, - FALSE, - "Not authorized to control networking.", - priv->user_data1, - priv->user_data2); - goto done; - } - - if (priv->wifi_shared_permission) { - result = nm_auth_chain_get_result (chain, priv->wifi_shared_permission); - if (result != NM_AUTH_CALL_RESULT_YES) { - priv->result_func (self, - FALSE, - "Not authorized to share connections via wifi.", - priv->user_data1, - priv->user_data2); - goto done; + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { + if (auth_call_id == priv->auth.call_id_network_control) + priv->auth.call_id_network_control = NULL; + else { + nm_assert (auth_call_id == priv->auth.call_id_wifi_shared_permission); + priv->auth.call_id_wifi_shared_permission = NULL; } + return; } - /* Otherwise authorized and available to activate */ - priv->result_func (self, TRUE, NULL, priv->user_data1, priv->user_data2); + result = nm_auth_call_result_eval (is_authorized, is_challenge, error); -done: - nm_auth_chain_destroy (chain); - priv->chain = NULL; - priv->result_func = NULL; - priv->user_data1 = NULL; - priv->user_data2 = NULL; + if (auth_call_id == priv->auth.call_id_network_control) { + priv->auth.call_id_network_control = NULL; + if (result != NM_AUTH_CALL_RESULT_YES) { + auth_complete (self, FALSE, "Not authorized to control networking."); + return; + } + } else { + nm_assert (auth_call_id == priv->auth.call_id_wifi_shared_permission); + if (result != NM_AUTH_CALL_RESULT_YES) { + /* we don't fail right away. Instead, we mark that wifi-shared-permissions + * are missing. We prefer to report the failure about network-control. + * Below, we will wait longer for call_id_network_control (if it's still + * pending). */ + priv->auth.call_id_wifi_shared_permission = AUTH_CALL_ID_SHARED_WIFI_PERMISSION_FAILED; + } else + priv->auth.call_id_wifi_shared_permission = NULL; + } - g_object_unref (self); + if (priv->auth.call_id_network_control) + return; + + if (priv->auth.call_id_wifi_shared_permission) { + if (priv->auth.call_id_wifi_shared_permission == AUTH_CALL_ID_SHARED_WIFI_PERMISSION_FAILED) + auth_complete (self, FALSE, "Not authorized to share connections via wifi."); + return; + } + + auth_complete (self, TRUE, NULL); } /** @@ -1064,8 +1102,9 @@ nm_active_connection_authorize (NMActiveConnection *self, const char *wifi_permission = NULL; NMConnection *con; - g_return_if_fail (result_func != NULL); - g_return_if_fail (priv->chain == NULL); + g_return_if_fail (result_func); + g_return_if_fail (!priv->auth.call_id_network_control); + nm_assert (!priv->auth.call_id_wifi_shared_permission); if (initial_connection) { g_return_if_fail (NM_IS_CONNECTION (initial_connection)); @@ -1078,23 +1117,28 @@ nm_active_connection_authorize (NMActiveConnection *self, con = priv->applied_connection; } - priv->chain = nm_auth_chain_new_subject (priv->subject, NULL, auth_done, self); - g_assert (priv->chain); - - /* Check that the subject is allowed to use networking at all */ - nm_auth_chain_add_call (priv->chain, NM_AUTH_PERMISSION_NETWORK_CONTROL, TRUE); + priv->auth.call_id_network_control = nm_auth_manager_check_authorization (nm_auth_manager_get (), + priv->subject, + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + auth_done, + self); /* Shared wifi connections require special permissions too */ wifi_permission = nm_utils_get_shared_wifi_permission (con); if (wifi_permission) { - priv->wifi_shared_permission = wifi_permission; - nm_auth_chain_add_call (priv->chain, wifi_permission, TRUE); + priv->auth.call_id_wifi_shared_permission = nm_auth_manager_check_authorization (nm_auth_manager_get (), + priv->subject, + wifi_permission, + TRUE, + auth_done, + self); } /* Wait for authorization */ - priv->result_func = result_func; - priv->user_data1 = user_data1; - priv->user_data2 = user_data2; + priv->auth.result_func = result_func; + priv->auth.user_data1 = user_data1; + priv->auth.user_data2 = user_data2; } /*****************************************************************************/ @@ -1387,10 +1431,7 @@ dispose (GObject *object) _LOGD ("disposing"); - if (priv->chain) { - nm_auth_chain_destroy (priv->chain); - priv->chain = NULL; - } + auth_cancel (self); g_free (priv->specific_object); priv->specific_object = NULL; From 457b08bbb6a0b19ecc05af57f8cb29ee974c8fe9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 Apr 2018 14:34:30 +0200 Subject: [PATCH 39/40] auth-chain/trivial: rename data field in NMAuthChain We already use "data" for other places. Let's use unique names that can be searched within one file. --- src/nm-auth-utils.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 4d268d0025..66daeede3a 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -34,7 +34,7 @@ /*****************************************************************************/ struct NMAuthChain { - GHashTable *data; + GHashTable *data_hash; CList auth_call_lst_head; @@ -119,7 +119,7 @@ _get_data (NMAuthChain *self, const char *tag) { ChainData *tmp; - tmp = g_hash_table_lookup (self->data, &tag); + tmp = g_hash_table_lookup (self->data_hash, &tag); return tmp ? tmp->data : NULL; } @@ -152,7 +152,7 @@ nm_auth_chain_steal_data (NMAuthChain *self, const char *tag) g_return_val_if_fail (self, NULL); g_return_val_if_fail (tag, NULL); - tmp = g_hash_table_lookup (self->data, &tag); + tmp = g_hash_table_lookup (self->data_hash, &tag); if (!tmp) return NULL; @@ -160,7 +160,7 @@ nm_auth_chain_steal_data (NMAuthChain *self, const char *tag) /* Make sure the destroy handler isn't called when freeing */ tmp->destroy = NULL; - g_hash_table_remove (self->data, tmp); + g_hash_table_remove (self->data_hash, tmp); return value; } @@ -174,9 +174,9 @@ nm_auth_chain_set_data (NMAuthChain *self, g_return_if_fail (tag); if (data == NULL) - g_hash_table_remove (self->data, &tag); + g_hash_table_remove (self->data_hash, &tag); else { - g_hash_table_add (self->data, + g_hash_table_add (self->data_hash, chain_data_new (tag, data, data_destroy)); } } @@ -331,7 +331,7 @@ nm_auth_chain_new_subject (NMAuthSubject *subject, self = g_slice_new0 (NMAuthChain); c_list_init (&self->auth_call_lst_head); self->refcount = 1; - self->data = g_hash_table_new_full (nm_pstr_hash, nm_pstr_equal, NULL, chain_data_free); + self->data_hash = g_hash_table_new_full (nm_pstr_hash, nm_pstr_equal, NULL, chain_data_free); self->done_func = done_func; self->user_data = user_data; self->context = context ? g_object_ref (context) : NULL; @@ -369,7 +369,7 @@ nm_auth_chain_destroy (NMAuthChain *self) while ((call = c_list_first_entry (&self->auth_call_lst_head, AuthCall, auth_call_lst))) auth_call_free (call); - nm_clear_pointer (&self->data, g_hash_table_destroy); + nm_clear_pointer (&self->data_hash, g_hash_table_destroy); g_slice_free (NMAuthChain, self); } From 04a42e2748478a6f6c204f94c7c83fa2c9b8dbd9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 Apr 2018 14:38:14 +0200 Subject: [PATCH 40/40] auth-chain: create data-hash hashtable only when needed It makes sense to use NMAuthChain also when not attaching any user-data to the chain. The main reason would be, the ability to schedule multiple permission checks in parallel, and wait for them to complete together. Only allocate the hash-table, when we really need it. --- src/nm-auth-utils.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 66daeede3a..cba118250f 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -119,6 +119,8 @@ _get_data (NMAuthChain *self, const char *tag) { ChainData *tmp; + if (!self->data_hash) + return NULL; tmp = g_hash_table_lookup (self->data_hash, &tag); return tmp ? tmp->data : NULL; } @@ -152,6 +154,9 @@ nm_auth_chain_steal_data (NMAuthChain *self, const char *tag) g_return_val_if_fail (self, NULL); g_return_val_if_fail (tag, NULL); + if (!self->data_hash) + return NULL; + tmp = g_hash_table_lookup (self->data_hash, &tag); if (!tmp) return NULL; @@ -173,9 +178,14 @@ nm_auth_chain_set_data (NMAuthChain *self, g_return_if_fail (self); g_return_if_fail (tag); - if (data == NULL) - g_hash_table_remove (self->data_hash, &tag); - else { + if (data == NULL) { + if (self->data_hash) + g_hash_table_remove (self->data_hash, &tag); + } else { + if (!self->data_hash) { + self->data_hash = g_hash_table_new_full (nm_pstr_hash, nm_pstr_equal, + NULL, chain_data_free); + } g_hash_table_add (self->data_hash, chain_data_new (tag, data, data_destroy)); } @@ -331,7 +341,6 @@ nm_auth_chain_new_subject (NMAuthSubject *subject, self = g_slice_new0 (NMAuthChain); c_list_init (&self->auth_call_lst_head); self->refcount = 1; - self->data_hash = g_hash_table_new_full (nm_pstr_hash, nm_pstr_equal, NULL, chain_data_free); self->done_func = done_func; self->user_data = user_data; self->context = context ? g_object_ref (context) : NULL;