From 4699a4c3cd17e6bc832d854386d47edc47bff331 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 6 Mar 2023 14:48:32 +0100 Subject: [PATCH 1/2] core/dbus: split RequestName D-Bus call out of initialization for NMDBusManager --- src/core/main.c | 13 +++--- src/core/nm-dbus-manager.c | 91 ++++++++++++++++++++++---------------- src/core/nm-dbus-manager.h | 4 +- 3 files changed, 63 insertions(+), 45 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 2eb230d9ca..4c436a631b 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -266,13 +266,14 @@ _dbus_manager_init(NMConfig *config) c_a_q_type = nm_config_get_configure_and_quit(config); - if (c_a_q_type == NM_CONFIG_CONFIGURE_AND_QUIT_DISABLED) - return nm_dbus_manager_acquire_bus(busmgr, TRUE); + if (c_a_q_type == NM_CONFIG_CONFIGURE_AND_QUIT_INITRD) { + /* in initrd we don't have D-Bus at all. Don't even try to get the G_BUS_TYPE_SYSTEM + * connection. And of course don't claim the D-Bus name. */ + return TRUE; + } - nm_assert(c_a_q_type == NM_CONFIG_CONFIGURE_AND_QUIT_INITRD); - /* in initrd we don't have D-Bus at all. Don't even try to get the G_BUS_TYPE_SYSTEM - * connection. And of course don't claim the D-Bus name. */ - return TRUE; + nm_assert(c_a_q_type == NM_CONFIG_CONFIGURE_AND_QUIT_DISABLED); + return nm_dbus_manager_setup(busmgr); } /* diff --git a/src/core/nm-dbus-manager.c b/src/core/nm-dbus-manager.c index af7de8c458..5ecd9fe680 100644 --- a/src/core/nm-dbus-manager.c +++ b/src/core/nm-dbus-manager.c @@ -1410,18 +1410,65 @@ nm_dbus_manager_start(NMDBusManager *self, } gboolean -nm_dbus_manager_acquire_bus(NMDBusManager *self, gboolean request_name) +nm_dbus_manager_request_name_sync(NMDBusManager *self) { NMDBusManagerPrivate *priv; gs_free_error GError *error = NULL; gs_unref_variant GVariant *ret = NULL; guint32 result; - guint registration_id; g_return_val_if_fail(NM_IS_DBUS_MANAGER(self), FALSE); priv = NM_DBUS_MANAGER_GET_PRIVATE(self); + g_return_val_if_fail(G_IS_DBUS_CONNECTION(priv->main_dbus_connection), FALSE); + + ret = g_dbus_connection_call_sync( + priv->main_dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "RequestName", + g_variant_new("(su)", NM_DBUS_SERVICE, DBUS_NAME_FLAG_DO_NOT_QUEUE), + G_VARIANT_TYPE("(u)"), + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + &error); + + if (!ret) { + _LOGE("fatal failure to acquire D-Bus service \"%s" + ": %s", + NM_DBUS_SERVICE, + error->message); + return FALSE; + } + + g_variant_get(ret, "(u)", &result); + if (result != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) { + _LOGE("fatal failure to acquire D-Bus service \"%s\" (%u). Service already taken", + NM_DBUS_SERVICE, + (guint) result); + return FALSE; + } + + _LOGI("acquired D-Bus service \"%s\"", NM_DBUS_SERVICE); + return TRUE; +} + +gboolean +nm_dbus_manager_setup(NMDBusManager *self) +{ + NMDBusManagerPrivate *priv; + gs_free_error GError *error = NULL; + guint registration_id; + + g_return_val_if_fail(NM_IS_DBUS_MANAGER(self), FALSE); + + priv = NM_DBUS_MANAGER_GET_PRIVATE(self); + + g_return_val_if_fail(!priv->main_dbus_connection, FALSE); + /* Create the D-Bus connection and registering the name synchronously. * That is necessary because we need to exit right away if we can't * acquire the name despite connecting to the bus successfully. @@ -1435,11 +1482,6 @@ nm_dbus_manager_acquire_bus(NMDBusManager *self, gboolean request_name) g_dbus_connection_set_exit_on_close(priv->main_dbus_connection, FALSE); - if (!request_name) { - _LOGD("D-Bus connection created"); - return TRUE; - } - registration_id = g_dbus_connection_register_object( priv->main_dbus_connection, OBJECT_MANAGER_SERVER_BASE_PATH, @@ -1453,39 +1495,12 @@ nm_dbus_manager_acquire_bus(NMDBusManager *self, gboolean request_name) return FALSE; } - ret = g_dbus_connection_call_sync( - priv->main_dbus_connection, - DBUS_SERVICE_DBUS, - DBUS_PATH_DBUS, - DBUS_INTERFACE_DBUS, - "RequestName", - g_variant_new("(su)", NM_DBUS_SERVICE, DBUS_NAME_FLAG_DO_NOT_QUEUE), - G_VARIANT_TYPE("(u)"), - G_DBUS_CALL_FLAGS_NONE, - -1, - NULL, - &error); - if (!ret) { - _LOGE("fatal failure to acquire D-Bus service \"%s" - ": %s", - NM_DBUS_SERVICE, - error->message); - g_dbus_connection_unregister_object(priv->main_dbus_connection, registration_id); - return FALSE; - } - - g_variant_get(ret, "(u)", &result); - if (result != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) { - _LOGE("fatal failure to acquire D-Bus service \"%s\" (%u). Service already taken", - NM_DBUS_SERVICE, - (guint) result); - g_dbus_connection_unregister_object(priv->main_dbus_connection, registration_id); - return FALSE; - } - priv->objmgr_registration_id = registration_id; - _LOGI("acquired D-Bus service \"%s\"", NM_DBUS_SERVICE); + _LOGD("D-Bus connection created and ObjectManager object registered"); + + if (!nm_dbus_manager_request_name_sync(self)) + return FALSE; return TRUE; } diff --git a/src/core/nm-dbus-manager.h b/src/core/nm-dbus-manager.h index b68161db88..078dbdd203 100644 --- a/src/core/nm-dbus-manager.h +++ b/src/core/nm-dbus-manager.h @@ -37,7 +37,9 @@ typedef void (*NMDBusManagerSetPropertyHandler)(NMDBusObject GVariant *value, gpointer user_data); -gboolean nm_dbus_manager_acquire_bus(NMDBusManager *self, gboolean request_name); +gboolean nm_dbus_manager_setup(NMDBusManager *self); + +gboolean nm_dbus_manager_request_name_sync(NMDBusManager *self); GDBusConnection *nm_dbus_manager_get_dbus_connection(NMDBusManager *self); From a6f5dbb426a9fa76cc2914d0cb685d98369bbce7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 6 Mar 2023 14:54:54 +0100 Subject: [PATCH 2/2] core/dbus: "RequestName" of NetworkManager D-Bus API later to fix race NetworkManager.service is "Type=dbus". Systemd takes that as indication for declaring the service as started when the D-Bus name is acquired. Currently, we acquire the name very early. The benefit is, that the service appears to start very fast. However, most the D-Bus API is not yet populated or ready to use. So if you order your service `After=NetworkManager.service`, then there is a race that NetworkManager might not yet be fully usable. Another benefit was that requesting a D-Bus name is atomic. That means, we could take that to ensure only one NetworkManager daemon was running. If we noticed that NetworkManager is already running, we would quit without doing anything. In practice, systemd already ensures that the daemon is not running in parallel. This was still useful for catching misuse when testing manually. This is now no longer done. We will notice a concurrent NetworkManager only very late, at which point we might have already broken things (e.g. rewrite wrong state files). Fix the race with `After=` by acquiring the name much later. Note that NetworkManager is pretty slow during initialization. This easily adds several hundreds of milliseconds to the startup. --- src/core/main.c | 3 +++ src/core/nm-dbus-manager.c | 8 +++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 4c436a631b..4c7de6cd37 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -508,6 +508,9 @@ main(int argc, char *argv[]) nm_log_dbg(LOGD_CORE, "setting up local loopback"); nm_platform_link_change_flags(NM_PLATFORM_GET, 1, IFF_UP, TRUE); + if (!nm_dbus_manager_request_name_sync(nm_dbus_manager_get())) + goto done; + success = TRUE; if (configure_and_quit == FALSE) { diff --git a/src/core/nm-dbus-manager.c b/src/core/nm-dbus-manager.c index 5ecd9fe680..0bde5971d8 100644 --- a/src/core/nm-dbus-manager.c +++ b/src/core/nm-dbus-manager.c @@ -1421,6 +1421,11 @@ nm_dbus_manager_request_name_sync(NMDBusManager *self) priv = NM_DBUS_MANAGER_GET_PRIVATE(self); + if (priv->objmgr_registration_id == 0) { + /* Do nothing. We're presumably in the configure-and-quit mode. */ + return TRUE; + } + g_return_val_if_fail(G_IS_DBUS_CONNECTION(priv->main_dbus_connection), FALSE); ret = g_dbus_connection_call_sync( @@ -1499,9 +1504,6 @@ nm_dbus_manager_setup(NMDBusManager *self) _LOGD("D-Bus connection created and ObjectManager object registered"); - if (!nm_dbus_manager_request_name_sync(self)) - return FALSE; - return TRUE; }