From 5e658530ab0d5f659661e4adb962dce6e61adb91 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 2 Aug 2021 19:27:48 +0200 Subject: [PATCH 01/28] glib-aux: add nm_io_sockaddr_un_set() helper --- src/libnm-glib-aux/nm-io-utils.c | 51 ++++++++++++++++++++++++++++++-- src/libnm-glib-aux/nm-io-utils.h | 15 ++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/libnm-glib-aux/nm-io-utils.c b/src/libnm-glib-aux/nm-io-utils.c index 87478a2dc2..a1e642cbaa 100644 --- a/src/libnm-glib-aux/nm-io-utils.c +++ b/src/libnm-glib-aux/nm-io-utils.c @@ -7,9 +7,11 @@ #include "nm-io-utils.h" -#include -#include #include +#include +#include +#include +#include #include "nm-str-buf.h" #include "nm-shared-utils.h" @@ -628,3 +630,48 @@ next:; g_ptr_array_add(arr, NULL); return (char **) g_ptr_array_free(arr, FALSE); } + +/*****************************************************************************/ + +/* taken from systemd's sockaddr_un_set_path(). */ +int +nm_io_sockaddr_un_set(struct sockaddr_un *ret, NMOptionBool is_abstract, const char *path) +{ + gsize l; + + g_return_val_if_fail(ret, -EINVAL); + g_return_val_if_fail(path, -EINVAL); + nm_assert_is_ternary(is_abstract); + + if (is_abstract == NM_OPTION_BOOL_DEFAULT) + is_abstract = nm_io_sockaddr_un_path_is_abstract(path, &path); + + l = strlen(path); + if (l < 1) + return -EINVAL; + if (l > sizeof(ret->sun_path) - 1) + return -EINVAL; + + if (!is_abstract) { + if (path[0] != '/') { + /* non-abstract paths must be absolute. */ + return -EINVAL; + } + } + + memset(ret, 0, nm_offsetof(struct sockaddr_un, sun_path)); + ret->sun_family = AF_UNIX; + + if (is_abstract) { + ret->sun_path[0] = '\0'; + memcpy(&ret->sun_path[1], path, NM_MIN(l + 1, sizeof(ret->sun_path) - 1)); + } else + memcpy(&ret->sun_path, path, l + 1); + + /* For pathname addresses, we return the size with the trailing NUL. + * For abstract addresses, we return the size without the trailing NUL + * (which may not be even written). But as abstract sockets also have + * a NUL at the beginning of sun_path, the total length is always + * calculated the same. */ + return (nm_offsetof(struct sockaddr_un, sun_path) + 1) + l; +} diff --git a/src/libnm-glib-aux/nm-io-utils.h b/src/libnm-glib-aux/nm-io-utils.h index 31ff6d0569..34d2c183e3 100644 --- a/src/libnm-glib-aux/nm-io-utils.h +++ b/src/libnm-glib-aux/nm-io-utils.h @@ -60,4 +60,19 @@ void nm_g_subprocess_terminate_in_background(GSubprocess *subprocess, int timeou char **nm_utils_find_mkstemp_files(const char *dirname, const char *filename); +static inline gboolean +nm_io_sockaddr_un_path_is_abstract(const char *path, const char **out_path) +{ + if (path && path[0] == '@') { + NM_SET_OUT(out_path, &path[1]); + return TRUE; + } + NM_SET_OUT(out_path, path); + return FALSE; +} + +struct sockaddr_un; + +int nm_io_sockaddr_un_set(struct sockaddr_un *ret, NMOptionBool is_abstract, const char *path); + #endif /* __NM_IO_UTILS_H__ */ From b87d7a8b402894ef8dfbb0af73a8eaef02660021 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 2 Aug 2021 21:56:39 +0200 Subject: [PATCH 02/28] glib-aux: add nm_sd_notify() helper Reimplements systemd's sd_notify(). We want to notify, but we don't want to link with libsystemd. --- src/libnm-glib-aux/nm-io-utils.c | 48 ++++++++++++++++++++++++++++++++ src/libnm-glib-aux/nm-io-utils.h | 2 ++ 2 files changed, 50 insertions(+) diff --git a/src/libnm-glib-aux/nm-io-utils.c b/src/libnm-glib-aux/nm-io-utils.c index a1e642cbaa..85a81f69bc 100644 --- a/src/libnm-glib-aux/nm-io-utils.c +++ b/src/libnm-glib-aux/nm-io-utils.c @@ -675,3 +675,51 @@ nm_io_sockaddr_un_set(struct sockaddr_un *ret, NMOptionBool is_abstract, const c * calculated the same. */ return (nm_offsetof(struct sockaddr_un, sun_path) + 1) + l; } + +/*****************************************************************************/ + +/* taken from systemd's sd_notify(). */ +int +nm_sd_notify(const char *state) +{ + struct sockaddr_un sockaddr; + struct iovec iovec; + struct msghdr msghdr = { + .msg_iov = &iovec, + .msg_iovlen = 1, + .msg_name = &sockaddr, + }; + nm_auto_close int fd = -1; + const char * e; + int r; + + if (!state) + g_return_val_if_reached(-EINVAL); + + e = getenv("NOTIFY_SOCKET"); + if (!e) + return 0; + + r = nm_io_sockaddr_un_set(&sockaddr, NM_OPTION_BOOL_DEFAULT, e); + if (r < 0) + return r; + msghdr.msg_namelen = r; + + fd = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0); + if (fd < 0) + return -NM_ERRNO_NATIVE(errno); + + /* systemd calls here fd_set_sndbuf(fd, SNDBUF_SIZE) .We don't bother. */ + + iovec = (struct iovec){ + .iov_base = (gpointer) state, + .iov_len = strlen(state), + }; + + /* systemd sends ucred, if geteuid()/getegid() does not match getuid()/getgid(). We don't bother. */ + + if (sendmsg(fd, &msghdr, MSG_NOSIGNAL) < 0) + return -NM_ERRNO_NATIVE(errno); + + return 0; +} diff --git a/src/libnm-glib-aux/nm-io-utils.h b/src/libnm-glib-aux/nm-io-utils.h index 34d2c183e3..479d0e5100 100644 --- a/src/libnm-glib-aux/nm-io-utils.h +++ b/src/libnm-glib-aux/nm-io-utils.h @@ -75,4 +75,6 @@ struct sockaddr_un; int nm_io_sockaddr_un_set(struct sockaddr_un *ret, NMOptionBool is_abstract, const char *path); +int nm_sd_notify(const char *state); + #endif /* __NM_IO_UTILS_H__ */ From 5d08d3a7ef5c7b5b745b9ed92549add4d7598ca7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 09:06:29 +0200 Subject: [PATCH 03/28] glib-aux: use GUnixFDSourceFunc for nm_g_unix_fd_source_new() signature --- src/libnm-glib-aux/nm-shared-utils.c | 12 ++++++------ src/libnm-glib-aux/nm-shared-utils.h | 14 +++++++------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/libnm-glib-aux/nm-shared-utils.c b/src/libnm-glib-aux/nm-shared-utils.c index d9928e324e..eb907445dd 100644 --- a/src/libnm-glib-aux/nm-shared-utils.c +++ b/src/libnm-glib-aux/nm-shared-utils.c @@ -5080,12 +5080,12 @@ nm_g_unix_signal_source_new(int signum, } GSource * -nm_g_unix_fd_source_new(int fd, - GIOCondition io_condition, - int priority, - gboolean (*source_func)(int fd, GIOCondition condition, gpointer user_data), - gpointer user_data, - GDestroyNotify destroy_notify) +nm_g_unix_fd_source_new(int fd, + GIOCondition io_condition, + int priority, + GUnixFDSourceFunc source_func, + gpointer user_data, + GDestroyNotify destroy_notify) { GSource *source; diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index d5894e82a6..1f2cc53c21 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -1715,13 +1715,13 @@ GSource *nm_g_timeout_source_new_seconds(guint timeout_sec, gpointer user_data, GDestroyNotify destroy_notify); -GSource * - nm_g_unix_fd_source_new(int fd, - GIOCondition io_condition, - int priority, - gboolean (*source_func)(int fd, GIOCondition condition, gpointer user_data), - gpointer user_data, - GDestroyNotify destroy_notify); +GSource *nm_g_unix_fd_source_new(int fd, + GIOCondition io_condition, + int priority, + GUnixFDSourceFunc source_func, + gpointer user_data, + GDestroyNotify destroy_notify); + GSource *nm_g_unix_signal_source_new(int signum, int priority, GSourceFunc handler, From b7c77d51eba3c3a3a6f89012b49dc173a3e8fbb5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 09:06:48 +0200 Subject: [PATCH 04/28] glib-aux: add nm_g_child_watch_source_new() and nm_g_child_watch_add_source() helpers --- src/libnm-glib-aux/nm-shared-utils.c | 17 +++++++++++++++++ src/libnm-glib-aux/nm-shared-utils.h | 14 ++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.c b/src/libnm-glib-aux/nm-shared-utils.c index eb907445dd..3fa00570b4 100644 --- a/src/libnm-glib-aux/nm-shared-utils.c +++ b/src/libnm-glib-aux/nm-shared-utils.c @@ -5097,6 +5097,23 @@ nm_g_unix_fd_source_new(int fd, return source; } +GSource * +nm_g_child_watch_source_new(GPid pid, + int priority, + GChildWatchFunc handler, + gpointer user_data, + GDestroyNotify notify) +{ + GSource *source; + + source = g_child_watch_source_new(pid); + + if (priority != G_PRIORITY_DEFAULT) + g_source_set_priority(source, priority); + g_source_set_callback(source, G_SOURCE_FUNC(handler), user_data, notify); + return source; +} + /*****************************************************************************/ #define _CTX_LOG(fmt, ...) \ diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 1f2cc53c21..4f80d973d7 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -1728,6 +1728,12 @@ GSource *nm_g_unix_signal_source_new(int signum, gpointer user_data, GDestroyNotify notify); +GSource *nm_g_child_watch_source_new(GPid pid, + int priority, + GChildWatchFunc handler, + gpointer user_data, + GDestroyNotify notify); + static inline GSource * nm_g_source_attach(GSource *source, GMainContext *context) { @@ -1824,6 +1830,14 @@ nm_g_unix_signal_add_source(int signum, GSourceFunc handler, gpointer user_data) NULL); } +static inline GSource * +nm_g_child_watch_add_source(GPid pid, GChildWatchFunc handler, gpointer user_data) +{ + return nm_g_source_attach( + nm_g_child_watch_source_new(pid, G_PRIORITY_DEFAULT, handler, user_data, NULL), + NULL); +} + NM_AUTO_DEFINE_FCN0(GMainContext *, _nm_auto_unref_gmaincontext, g_main_context_unref); #define nm_auto_unref_gmaincontext nm_auto(_nm_auto_unref_gmaincontext) From 133dc3d43c8c04fcef47f9702c9fcb10305306fb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 10:07:13 +0200 Subject: [PATCH 05/28] glib-aux: add nm_g_bus_get_blocking() helper --- src/libnm-glib-aux/nm-dbus-aux.c | 50 ++++++++++++++++++++++++++++++++ src/libnm-glib-aux/nm-dbus-aux.h | 2 ++ 2 files changed, 52 insertions(+) diff --git a/src/libnm-glib-aux/nm-dbus-aux.c b/src/libnm-glib-aux/nm-dbus-aux.c index f4f53a7d58..4c570885d2 100644 --- a/src/libnm-glib-aux/nm-dbus-aux.c +++ b/src/libnm-glib-aux/nm-dbus-aux.c @@ -403,3 +403,53 @@ _nm_dbus_error_is(GError *error, ...) return FALSE; } + +/*****************************************************************************/ + +typedef struct { + GDBusConnection **p_dbus_connection; + GError ** p_error; +} BusGetData; + +static void +_bus_get_cb(GObject *source, GAsyncResult *result, gpointer user_data) +{ + BusGetData *data = user_data; + + *data->p_dbus_connection = g_bus_get_finish(result, data->p_error); +} + +/** + * nm_g_bus_get_blocking: + * @cancellable: (allow-none): a #GCancellable to abort the operation. + * @error: (allow-none): the error. + * + * This calls g_bus_get(), but iterates the current (thread-default) GMainContext + * until the response is ready. As such, it's similar to g_bus_get_sync(), + * but it allows to cancel the operation (without having multiple threads). + * + * Returns: (transfer full): the new #GDBusConnection or %NULL on error. + */ +GDBusConnection * +nm_g_bus_get_blocking(GCancellable *cancellable, GError **error) +{ + gs_free_error GError *local_error = NULL; + gs_unref_object GDBusConnection *dbus_connection = NULL; + GMainContext * main_context = g_main_context_get_thread_default(); + BusGetData data = { + .p_dbus_connection = &dbus_connection, + .p_error = &local_error, + }; + + g_bus_get(G_BUS_TYPE_SYSTEM, cancellable, _bus_get_cb, &data); + + while (!dbus_connection && !local_error) + g_main_context_iteration(main_context, TRUE); + + if (!dbus_connection) { + g_propagate_error(error, g_steal_pointer(&local_error)); + return NULL; + } + + return g_steal_pointer(&dbus_connection); +} diff --git a/src/libnm-glib-aux/nm-dbus-aux.h b/src/libnm-glib-aux/nm-dbus-aux.h index 65a91311e7..ea86a14ecc 100644 --- a/src/libnm-glib-aux/nm-dbus-aux.h +++ b/src/libnm-glib-aux/nm-dbus-aux.h @@ -215,4 +215,6 @@ gboolean _nm_dbus_error_is(GError *error, ...) G_GNUC_NULL_TERMINATED; /*****************************************************************************/ +GDBusConnection *nm_g_bus_get_blocking(GCancellable *cancellable, GError **error); + #endif /* __NM_DBUS_AUX_H__ */ From 68e049119aa304f88e5a0af25c770df92f37d3e5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 11:33:09 +0200 Subject: [PATCH 06/28] glib-aux: add nm_g_main_context_iterate_ready() helper --- src/libnm-glib-aux/nm-shared-utils.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 4f80d973d7..ef4d3937f1 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -1893,6 +1893,14 @@ nm_g_main_context_push_thread_default_if_necessary(GMainContext *context) return context; } +static inline void +nm_g_main_context_iterate_ready(GMainContext *context) +{ + while (g_main_context_iteration(context, FALSE)) { + ; + } +} + /*****************************************************************************/ static inline int From dc2e0d30bb784345231ccf328d44000d3e2b47a2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 13:09:19 +0200 Subject: [PATCH 07/28] glib-aux: add nm_dbus_connection_call_request_name() helper --- src/libnm-glib-aux/nm-dbus-aux.h | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/libnm-glib-aux/nm-dbus-aux.h b/src/libnm-glib-aux/nm-dbus-aux.h index ea86a14ecc..7e082645a1 100644 --- a/src/libnm-glib-aux/nm-dbus-aux.h +++ b/src/libnm-glib-aux/nm-dbus-aux.h @@ -84,6 +84,29 @@ void nm_dbus_connection_call_get_name_owner(GDBusConnection * d NMDBusConnectionCallGetNameOwnerCb callback, gpointer user_data); +static inline void +nm_dbus_connection_call_request_name(GDBusConnection * dbus_connection, + const char * name, + guint32 flags, + int timeout_msec, + GCancellable * cancellable, + GAsyncReadyCallback callback, + gpointer user_data) +{ + g_dbus_connection_call(dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "RequestName", + g_variant_new("(su)", name, flags), + G_VARIANT_TYPE("(u)"), + G_DBUS_CALL_FLAGS_NONE, + timeout_msec, + cancellable, + callback, + user_data); +} + static inline guint nm_dbus_connection_signal_subscribe_properties_changed(GDBusConnection * dbus_connection, const char * bus_name, From 864bfb4052372ffe487125b508ff82c7216ec53a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 13:24:42 +0200 Subject: [PATCH 08/28] glib-aux: add nm_dbus_connection_call_blocking() helper --- src/libnm-glib-aux/nm-dbus-aux.c | 37 ++++++++++++++++++++++++++++++++ src/libnm-glib-aux/nm-dbus-aux.h | 12 +++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/libnm-glib-aux/nm-dbus-aux.c b/src/libnm-glib-aux/nm-dbus-aux.c index 4c570885d2..454bd2d8c6 100644 --- a/src/libnm-glib-aux/nm-dbus-aux.c +++ b/src/libnm-glib-aux/nm-dbus-aux.c @@ -453,3 +453,40 @@ nm_g_bus_get_blocking(GCancellable *cancellable, GError **error) return g_steal_pointer(&dbus_connection); } + +/*****************************************************************************/ + +void +nm_dbus_connection_call_blocking_callback(GObject *source, GAsyncResult *res, gpointer user_data) +{ + NMDBusConnectionCallBlockingData *data = user_data; + + nm_assert(data); + nm_assert(!data->result); + nm_assert(!data->error); + + data->result = g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), res, &data->error); +} + +GVariant * +nm_dbus_connection_call_blocking(NMDBusConnectionCallBlockingData *data, GError **error) +{ + GMainContext *main_context = g_main_context_get_thread_default(); + gs_free_error GError *local_error = NULL; + gs_unref_variant GVariant *result = NULL; + + nm_assert(data); + + while (!data->result && !data->error) + g_main_context_iteration(main_context, TRUE); + + local_error = g_steal_pointer(&data->error); + result = g_steal_pointer(&data->result); + + if (!result) { + g_propagate_error(error, g_steal_pointer(&local_error)); + return NULL; + } + + return g_steal_pointer(&result); +} diff --git a/src/libnm-glib-aux/nm-dbus-aux.h b/src/libnm-glib-aux/nm-dbus-aux.h index 7e082645a1..f71b05b568 100644 --- a/src/libnm-glib-aux/nm-dbus-aux.h +++ b/src/libnm-glib-aux/nm-dbus-aux.h @@ -240,4 +240,16 @@ gboolean _nm_dbus_error_is(GError *error, ...) G_GNUC_NULL_TERMINATED; GDBusConnection *nm_g_bus_get_blocking(GCancellable *cancellable, GError **error); +/*****************************************************************************/ + +typedef struct { + GVariant *result; + GError * error; +} NMDBusConnectionCallBlockingData; + +void +nm_dbus_connection_call_blocking_callback(GObject *source, GAsyncResult *res, gpointer user_data); + +GVariant *nm_dbus_connection_call_blocking(NMDBusConnectionCallBlockingData *data, GError **error); + #endif /* __NM_DBUS_AUX_H__ */ From 292cf4c42f89b86a158aefab00987b689a739091 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 09:43:31 +0200 Subject: [PATCH 09/28] nm-sudo: drop semicolon after _nm_log() macro --- src/nm-sudo/nm-sudo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nm-sudo/nm-sudo.c b/src/nm-sudo/nm-sudo.c index dd1581de18..94dcabe321 100644 --- a/src/nm-sudo/nm-sudo.c +++ b/src/nm-sudo/nm-sudo.c @@ -66,7 +66,7 @@ static void _pending_job_register_object(GlobalData *gl, GObject *obj); /*****************************************************************************/ -#define _nm_log(level, ...) _nm_log_simple_printf((level), __VA_ARGS__); +#define _nm_log(level, ...) _nm_log_simple_printf((level), __VA_ARGS__) #define _NMLOG(level, ...) \ G_STMT_START \ From 5d9a46ad34b3e887cc76bd0403910f88193e5d4a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 2 Aug 2021 19:33:18 +0200 Subject: [PATCH 10/28] nm-sudo: use nm_io_sockaddr_un_set() in nm_sudo_utils_open_fd() --- src/libnm-base/nm-sudo-utils.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/libnm-base/nm-sudo-utils.c b/src/libnm-base/nm-sudo-utils.c index 18bd0051bd..fd9bac94f6 100644 --- a/src/libnm-base/nm-sudo-utils.c +++ b/src/libnm-base/nm-sudo-utils.c @@ -7,6 +7,8 @@ #include #include +#include "libnm-glib-aux/nm-io-utils.h" + /*****************************************************************************/ int @@ -20,13 +22,11 @@ nm_sudo_utils_open_fd(NMSudoGetFDType fd_type, GError **error) case NM_SUDO_GET_FD_TYPE_OVSDB_SOCKET: { struct sockaddr_un sock; + int sock_len; - memset(&sock, 0, sizeof(sock)); - sock.sun_family = AF_UNIX; G_STATIC_ASSERT_EXPR(NM_STRLEN(NM_OVSDB_SOCKET) + 1 < sizeof(sock.sun_path)); - if (g_strlcpy(sock.sun_path, NM_OVSDB_SOCKET, sizeof(sock.sun_path)) - >= sizeof(sock.sun_path)) - nm_assert_not_reached(); + sock_len = nm_io_sockaddr_un_set(&sock, FALSE, NM_OVSDB_SOCKET); + nm_assert(sock_len > 0); fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); if (fd < 0) { @@ -35,9 +35,7 @@ nm_sudo_utils_open_fd(NMSudoGetFDType fd_type, GError **error) return -errsv; } - r = connect(fd, - (const struct sockaddr *) &sock, - G_STRUCT_OFFSET(struct sockaddr_un, sun_path) + NM_STRLEN(NM_OVSDB_SOCKET) + 1); + r = connect(fd, (const struct sockaddr *) &sock, sock_len); if (r != 0) { errsv = NM_ERRNO_NATIVE(errno); g_set_error(error, From a210e9a6f40e8a438bf9559378c4352034154005 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 2 Aug 2021 22:30:47 +0200 Subject: [PATCH 11/28] nm-sudo: fix race during exit-on-idle nm-sudo is D-Bus activated and exits-on-idle. To do that race-free we need: - sd_notify("STOPPING=1") - ReleaseName - keep processing pending requests --- data/nm-sudo.service.in | 1 + src/nm-sudo/nm-sudo.c | 71 +++++++++++++++++++++++++++++++++++------ 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/data/nm-sudo.service.in b/data/nm-sudo.service.in index 4ed8b67ba9..22abdd236b 100644 --- a/data/nm-sudo.service.in +++ b/data/nm-sudo.service.in @@ -18,6 +18,7 @@ Description=NetworkManager Sudo Helper Type=dbus BusName=org.freedesktop.nm.sudo ExecStart=@libexecdir@/nm-sudo +NotifyAccess=main # Extra configuration options. Set via `systemctl edit nm-sudo.service`: # diff --git a/src/nm-sudo/nm-sudo.c b/src/nm-sudo/nm-sudo.c index 94dcabe321..964072e9a9 100644 --- a/src/nm-sudo/nm-sudo.c +++ b/src/nm-sudo/nm-sudo.c @@ -5,10 +5,11 @@ #include #include "c-list/src/c-list.h" +#include "libnm-glib-aux/nm-dbus-aux.h" +#include "libnm-glib-aux/nm-io-utils.h" #include "libnm-glib-aux/nm-logging-base.h" #include "libnm-glib-aux/nm-shared-utils.h" #include "libnm-glib-aux/nm-time-utils.h" -#include "libnm-glib-aux/nm-dbus-aux.h" #include "libnm-base/nm-sudo-utils.h" /* nm-sudo doesn't link with libnm-core nor libnm-base, but these headers @@ -50,6 +51,7 @@ struct _GlobalData { guint32 timeout_msec; bool name_owner_initialized; bool service_registered; + bool name_requested; /* This is controlled by $NM_SUDO_NO_AUTH_FOR_TESTING. It disables authentication * of the request, so it is ONLY for testing. */ @@ -401,6 +403,10 @@ _bus_register_service(GlobalData *gl) .is_waiting = TRUE, }; + /* regardless whether the request is successful, after we start calling + * RequestName, we remember that we need to ReleaseName it. */ + gl->name_requested = TRUE; + g_dbus_connection_call( gl->dbus_connection, DBUS_SERVICE_DBUS, @@ -412,7 +418,7 @@ _bus_register_service(GlobalData *gl) (guint) (DBUS_NAME_FLAG_ALLOW_REPLACEMENT | DBUS_NAME_FLAG_REPLACE_EXISTING)), G_VARIANT_TYPE("(u)"), G_DBUS_CALL_FLAGS_NONE, - -1, + 10000, gl->quit_cancellable, _bus_register_service_request_name_cb, &data); @@ -505,6 +511,16 @@ _pending_job_register_object(GlobalData *gl, GObject *obj) /*****************************************************************************/ +static void +_bus_release_name_cb(GObject *source, GAsyncResult *result, gpointer user_data) +{ + _nm_unused gs_unref_object GObject *keep_alive_object = user_data; + + g_main_context_wakeup(NULL); +} + +/*****************************************************************************/ + static void _initial_setup(GlobalData *gl) { @@ -582,14 +598,49 @@ main(int argc, char **argv) while (TRUE) { if (!c_list_is_empty(&gl->pending_jobs_lst_head)) { /* we must first reply to all requests. No matter what. */ - } else { - if (gl->is_shutting_down_quitting || gl->is_shutting_down_timeout) { - /* we either hit the idle timeout or received SIGTERM. Note that - * if we received an idle-timeout and the very moment afterwards - * a new request, then _bus_method_call() will clear gl->is_shutting_down_timeout - * (via _pending_job_register_object()). */ - break; + } else if (gl->is_shutting_down_quitting || gl->is_shutting_down_timeout) { + /* we either hit the idle timeout or received SIGTERM. Note that + * if we received an idle-timeout and the very moment afterwards + * a new request, then _bus_method_call() will clear gl->is_shutting_down_timeout + * (via _pending_job_register_object()). */ + + if (gl->name_requested) { + gs_unref_object GObject *keep_alive_object = g_object_new(G_TYPE_OBJECT, NULL); + + /* We already requested a name. To exit-on-idle without race, we need to dance. + * See https://lists.freedesktop.org/archives/dbus/2015-May/016671.html . */ + + gl->name_requested = FALSE; + gl->is_shutting_down_quitting = TRUE; + + _LOGT("shutdown: release-name"); + + /* we use the _pending_job_register_object() mechanism to make the loop busy during + * shutdown. */ + _pending_job_register_object(gl, keep_alive_object); + + r = nm_sd_notify("STOPPING=1"); + if (r < 0) + _LOGW("shutdown: sd_notifiy(STOPPING=1) failed: %s", nm_strerror_native(-r)); + else + _LOGT("shutdown: sd_notifiy(STOPPING=1) succeeded"); + + g_dbus_connection_call(gl->dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "ReleaseName", + g_variant_new("(s)", NM_SUDO_DBUS_BUS_NAME), + G_VARIANT_TYPE("(u)"), + G_DBUS_CALL_FLAGS_NONE, + 10000, + NULL, + _bus_release_name_cb, + g_steal_pointer(&keep_alive_object)); + continue; } + + break; } g_main_context_iteration(NULL, TRUE); @@ -597,7 +648,7 @@ main(int argc, char **argv) done: gl->is_shutting_down_cleanup = TRUE; - _LOGD("exiting..."); + _LOGD("shutdown: cleanup"); nm_assert(c_list_is_empty(&gl->pending_jobs_lst_head)); From 62a9a48cc27c8d301e11e33c179eb736e6bdc8a1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 10:08:30 +0200 Subject: [PATCH 12/28] nm-sudo: use nm_g_bus_get_blocking() to create GDBusConnection --- src/nm-sudo/nm-sudo.c | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/src/nm-sudo/nm-sudo.c b/src/nm-sudo/nm-sudo.c index 964072e9a9..2c1b141651 100644 --- a/src/nm-sudo/nm-sudo.c +++ b/src/nm-sudo/nm-sudo.c @@ -142,33 +142,13 @@ _signal_callback_term(gpointer user_data) /*****************************************************************************/ -typedef struct { - GDBusConnection **p_dbus_connection; - GError ** p_error; -} BusGetData; - -static void -_bus_get_cb(GObject *source, GAsyncResult *result, gpointer user_data) -{ - BusGetData *data = user_data; - - *data->p_dbus_connection = g_bus_get_finish(result, data->p_error); -} - static GDBusConnection * _bus_get(GCancellable *cancellable, int *out_exit_code) { gs_free_error GError *error = NULL; gs_unref_object GDBusConnection *dbus_connection = NULL; - BusGetData data = { - .p_dbus_connection = &dbus_connection, - .p_error = &error, - }; - g_bus_get(G_BUS_TYPE_SYSTEM, cancellable, _bus_get_cb, &data); - - while (!dbus_connection && !error) - g_main_context_iteration(NULL, TRUE); + dbus_connection = nm_g_bus_get_blocking(cancellable, &error); if (!dbus_connection) { gboolean was_cancelled = nm_utils_error_is_cancelled(error); From 2b8add959fb46b3ec3a83c40d311056988de3e18 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 11:29:07 +0200 Subject: [PATCH 13/28] nm-sudo: cancel quit_cancellable during shutdown --- src/nm-sudo/nm-sudo.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/nm-sudo/nm-sudo.c b/src/nm-sudo/nm-sudo.c index 2c1b141651..fe58a90b0d 100644 --- a/src/nm-sudo/nm-sudo.c +++ b/src/nm-sudo/nm-sudo.c @@ -627,9 +627,11 @@ main(int argc, char **argv) } done: - gl->is_shutting_down_cleanup = TRUE; _LOGD("shutdown: cleanup"); + gl->is_shutting_down_cleanup = TRUE; + g_cancellable_cancel(gl->quit_cancellable); + nm_assert(c_list_is_empty(&gl->pending_jobs_lst_head)); if (gl->service_regist_id != 0) { @@ -640,7 +642,6 @@ done: g_dbus_connection_signal_unsubscribe(gl->dbus_connection, nm_steal_int(&gl->name_owner_changed_id)); } - nm_clear_g_cancellable(&gl->quit_cancellable); nm_clear_g_source_inst(&gl->source_sigterm); nm_clear_g_source_inst(&gl->source_idle_timeout); nm_clear_g_free(&gl->name_owner); @@ -658,6 +659,8 @@ done: } } + nm_clear_g_cancellable(&gl->quit_cancellable); + _LOGD("exit (%d)", exit_code); return exit_code; } From eeb01bcba9486bab14a8a5b48f00222246dcfefd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 11:35:02 +0200 Subject: [PATCH 14/28] nm-sudo: use nm_g_main_context_iterate_ready() helper --- src/nm-sudo/nm-sudo.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/nm-sudo/nm-sudo.c b/src/nm-sudo/nm-sudo.c index fe58a90b0d..6c0f9191c5 100644 --- a/src/nm-sudo/nm-sudo.c +++ b/src/nm-sudo/nm-sudo.c @@ -646,17 +646,12 @@ done: nm_clear_g_source_inst(&gl->source_idle_timeout); nm_clear_g_free(&gl->name_owner); - while (g_main_context_iteration(NULL, FALSE)) { - ; - } + nm_g_main_context_iterate_ready(NULL); if (gl->dbus_connection) { g_dbus_connection_flush_sync(gl->dbus_connection, NULL, NULL); g_clear_object(&gl->dbus_connection); - - while (g_main_context_iteration(NULL, FALSE)) { - ; - } + nm_g_main_context_iterate_ready(NULL); } nm_clear_g_cancellable(&gl->quit_cancellable); From 510599551466663bbed1ed6d71090f823a90f398 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 13:32:10 +0200 Subject: [PATCH 15/28] nm-sudo: use nm_dbus_connection_call_blocking() in _bus_register_service() --- src/nm-sudo/nm-sudo.c | 102 +++++++++++++++--------------------------- 1 file changed, 37 insertions(+), 65 deletions(-) diff --git a/src/nm-sudo/nm-sudo.c b/src/nm-sudo/nm-sudo.c index 6c0f9191c5..56d7e6755a 100644 --- a/src/nm-sudo/nm-sudo.c +++ b/src/nm-sudo/nm-sudo.c @@ -312,54 +312,18 @@ static GDBusInterfaceInfo *const interface_info = NM_DEFINE_GDBUS_INTERFACE_INFO .in_args = NM_DEFINE_GDBUS_ARG_INFOS( NM_DEFINE_GDBUS_ARG_INFO("fd_type", "u"), ), ), ), ); -typedef struct { - GlobalData *gl; - gboolean is_waiting; -} BusRegisterServiceRequestNameData; - -static void -_bus_register_service_request_name_cb(GObject *source, GAsyncResult *res, gpointer user_data) -{ - BusRegisterServiceRequestNameData *data = user_data; - gs_free_error GError *error = NULL; - gs_unref_variant GVariant *ret = NULL; - gboolean success = FALSE; - - ret = g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), res, &error); - - if (nm_utils_error_is_cancelled(error)) - goto out; - - if (error) - _LOGE("d-bus: failed to request name %s: %s", NM_SUDO_DBUS_BUS_NAME, error->message); - else { - guint32 ret_val; - - g_variant_get(ret, "(u)", &ret_val); - if (ret_val != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) { - _LOGW("dbus: request name for %s failed to take name (response %u)", - NM_SUDO_DBUS_BUS_NAME, - ret_val); - } else { - _LOGD("dbus: request name for %s succeeded", NM_SUDO_DBUS_BUS_NAME); - success = TRUE; - } - } - -out: - if (success) - data->gl->service_registered = TRUE; - data->is_waiting = FALSE; -} - static void _bus_register_service(GlobalData *gl) { static const GDBusInterfaceVTable interface_vtable = { .method_call = _bus_method_call, }; - gs_free_error GError * error = NULL; - BusRegisterServiceRequestNameData data; + gs_free_error GError * error = NULL; + NMDBusConnectionCallBlockingData data = { + .result = NULL, + }; + gs_unref_variant GVariant *ret = NULL; + guint32 ret_val; nm_assert(!gl->service_registered); @@ -378,36 +342,44 @@ _bus_register_service(GlobalData *gl) _LOGD("dbus: object %s registered", NM_SUDO_DBUS_OBJECT_PATH); - data = (BusRegisterServiceRequestNameData){ - .gl = gl, - .is_waiting = TRUE, - }; - /* regardless whether the request is successful, after we start calling * RequestName, we remember that we need to ReleaseName it. */ gl->name_requested = TRUE; - g_dbus_connection_call( - gl->dbus_connection, - DBUS_SERVICE_DBUS, - DBUS_PATH_DBUS, - DBUS_INTERFACE_DBUS, - "RequestName", - g_variant_new("(su)", - NM_SUDO_DBUS_BUS_NAME, - (guint) (DBUS_NAME_FLAG_ALLOW_REPLACEMENT | DBUS_NAME_FLAG_REPLACE_EXISTING)), - G_VARIANT_TYPE("(u)"), - G_DBUS_CALL_FLAGS_NONE, - 10000, - gl->quit_cancellable, - _bus_register_service_request_name_cb, - &data); + nm_dbus_connection_call_request_name(gl->dbus_connection, + NM_SUDO_DBUS_BUS_NAME, + DBUS_NAME_FLAG_ALLOW_REPLACEMENT + | DBUS_NAME_FLAG_REPLACE_EXISTING, + 10000, + gl->quit_cancellable, + nm_dbus_connection_call_blocking_callback, + &data); /* Note that with D-Bus activation, the first request will already hit us before RequestName - * completes. */ + * completes. So when we start iterating the main context, the first request may already come + * in. */ - while (data.is_waiting) - g_main_context_iteration(NULL, TRUE); + ret = nm_dbus_connection_call_blocking(&data, &error); + + if (nm_utils_error_is_cancelled(error)) + return; + + if (error) { + _LOGE("d-bus: failed to request name %s: %s", NM_SUDO_DBUS_BUS_NAME, error->message); + return; + } + + g_variant_get(ret, "(u)", &ret_val); + + if (ret_val != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) { + _LOGW("dbus: request name for %s failed to take name (response %u)", + NM_SUDO_DBUS_BUS_NAME, + ret_val); + return; + } + + _LOGD("dbus: request name for %s succeeded", NM_SUDO_DBUS_BUS_NAME); + gl->service_registered = TRUE; } /*****************************************************************************/ From 1e71a0081719d9cb6fe35e681489239c714a151f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 13:37:09 +0200 Subject: [PATCH 16/28] nm-sudo: return result from _bus_register_service() Instead of adding a flag to global state. --- src/nm-sudo/nm-sudo.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/nm-sudo/nm-sudo.c b/src/nm-sudo/nm-sudo.c index 56d7e6755a..c199d8994b 100644 --- a/src/nm-sudo/nm-sudo.c +++ b/src/nm-sudo/nm-sudo.c @@ -50,7 +50,6 @@ struct _GlobalData { gint64 start_timestamp_msec; guint32 timeout_msec; bool name_owner_initialized; - bool service_registered; bool name_requested; /* This is controlled by $NM_SUDO_NO_AUTH_FOR_TESTING. It disables authentication @@ -312,7 +311,7 @@ static GDBusInterfaceInfo *const interface_info = NM_DEFINE_GDBUS_INTERFACE_INFO .in_args = NM_DEFINE_GDBUS_ARG_INFOS( NM_DEFINE_GDBUS_ARG_INFO("fd_type", "u"), ), ), ), ); -static void +static gboolean _bus_register_service(GlobalData *gl) { static const GDBusInterfaceVTable interface_vtable = { @@ -325,8 +324,6 @@ _bus_register_service(GlobalData *gl) gs_unref_variant GVariant *ret = NULL; guint32 ret_val; - nm_assert(!gl->service_registered); - gl->service_regist_id = g_dbus_connection_register_object(gl->dbus_connection, NM_SUDO_DBUS_OBJECT_PATH, @@ -337,7 +334,7 @@ _bus_register_service(GlobalData *gl) &error); if (gl->service_regist_id == 0) { _LOGE("dbus: error registering object %s: %s", NM_SUDO_DBUS_OBJECT_PATH, error->message); - return; + return FALSE; } _LOGD("dbus: object %s registered", NM_SUDO_DBUS_OBJECT_PATH); @@ -362,11 +359,11 @@ _bus_register_service(GlobalData *gl) ret = nm_dbus_connection_call_blocking(&data, &error); if (nm_utils_error_is_cancelled(error)) - return; + return FALSE; if (error) { _LOGE("d-bus: failed to request name %s: %s", NM_SUDO_DBUS_BUS_NAME, error->message); - return; + return FALSE; } g_variant_get(ret, "(u)", &ret_val); @@ -375,11 +372,11 @@ _bus_register_service(GlobalData *gl) _LOGW("dbus: request name for %s failed to take name (response %u)", NM_SUDO_DBUS_BUS_NAME, ret_val); - return; + return FALSE; } _LOGD("dbus: request name for %s succeeded", NM_SUDO_DBUS_BUS_NAME); - gl->service_registered = TRUE; + return TRUE; } /*****************************************************************************/ @@ -536,14 +533,14 @@ main(int argc, char **argv) exit_code = EXIT_SUCCESS; - _bus_register_service(gl); - if (!gl->service_registered) { + if (!_bus_register_service(gl)) { /* We failed to RequestName, but due to D-Bus activation we * might have a pending request still (on the unique name). * Process it below. * * Let's fake a shutdown signal, and still process the request below. */ - exit_code = EXIT_FAILURE; + if (!g_cancellable_is_cancelled(gl->quit_cancellable)) + exit_code = EXIT_FAILURE; gl->is_shutting_down_quitting = TRUE; } From dbd459ec9253fe97441e7c54b136c929074c4762 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 18:07:50 +0200 Subject: [PATCH 17/28] nm-sudo: expect unknown interface in _bus_method_call() GDBus will invoke the method_call callback also for the Get/Set functions. Thus, we need to check the interface_name and handle them (actually, there is nothing to handle, no properties exist). Also, "Ping" method only exists for testing. It is usually not called in production, so check for "GetFD" first. --- src/nm-sudo/nm-sudo.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/nm-sudo/nm-sudo.c b/src/nm-sudo/nm-sudo.c index c199d8994b..d8cf42f3c8 100644 --- a/src/nm-sudo/nm-sudo.c +++ b/src/nm-sudo/nm-sudo.c @@ -290,14 +290,26 @@ _bus_method_call(GDBusConnection * connection, method_name, g_variant_get_type_string(parameters)); + if (!nm_streq(interface_name, NM_SUDO_DBUS_IFACE_NAME)) + goto out_unknown_method; + + if (nm_streq(method_name, "GetFD")) { + g_variant_get(parameters, "(u)", &arg_u); + _handle_get_fd(gl, invocation, arg_u); + return; + } if (nm_streq(method_name, "Ping")) { g_variant_get(parameters, "(&s)", &arg_s); _handle_ping(gl, invocation, arg_s); - } else if (nm_streq(method_name, "GetFD")) { - g_variant_get(parameters, "(u)", &arg_u); - _handle_get_fd(gl, invocation, arg_u); - } else - nm_assert_not_reached(); + return; + } + +out_unknown_method: + g_dbus_method_invocation_return_error(invocation, + G_DBUS_ERROR, + G_DBUS_ERROR_UNKNOWN_METHOD, + "Unknown method %s", + method_name); } static GDBusInterfaceInfo *const interface_info = NM_DEFINE_GDBUS_INTERFACE_INFO( From 82174a66c60f978dd8ae7c4c39d9100bb914befe Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 2 Aug 2021 22:54:35 +0200 Subject: [PATCH 18/28] dispatcher: add comment about exit-on-idle race --- src/nm-dispatcher/nm-dispatcher.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index 6751a2cb0b..b32ee886b3 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -1054,6 +1054,9 @@ main(int argc, char **argv) done: + /* FIXME: nm-dispatcher does not exit-on-idle in a racefree manner. + * See https://lists.freedesktop.org/archives/dbus/2015-May/016671.html */ + if (gl.num_requests_pending > 0) { /* this only happens when we quit due to SIGTERM (not due to the idle timer). * From 7b4cb01366806cabda7500f715e1324988b690ce Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 08:49:04 +0200 Subject: [PATCH 19/28] dispatcher: replace GMainLoop by explicit context iteration Explicitly iterating the context is more flexible, as we can control the parameters how long we iterate. GMainLoop is essentially a (thread-safe) iteration around one boolean flag (controlled by g_main_loop_run() and g_main_loop_quit()). We can maintain that boolean flag ourselves. --- src/nm-dispatcher/nm-dispatcher.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index b32ee886b3..923e06b957 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -26,7 +26,6 @@ typedef struct Request Request; static struct { GDBusConnection *dbus_connection; - GMainLoop * loop; gboolean debug; gboolean persist; guint quit_id; @@ -34,6 +33,9 @@ static struct { gboolean ever_acquired_name; bool exit_with_failure; + bool shutdown_timeout; + bool shutdown_quitting; + Request *current_request; GQueue * requests_waiting; int num_requests_pending; @@ -189,8 +191,8 @@ request_free(Request *request) static gboolean quit_timeout_cb(gpointer user_data) { - gl.quit_id = 0; - g_main_loop_quit(gl.loop); + gl.quit_id = 0; + gl.shutdown_timeout = TRUE; return G_SOURCE_REMOVE; } @@ -851,7 +853,7 @@ on_name_lost(GDBusConnection *connection, const char *name, gpointer user_data) } else _LOG_X_I("Lost the " NM_DISPATCHER_DBUS_SERVICE " name. Exiting"); - g_main_loop_quit(gl.loop); + gl.shutdown_quitting = TRUE; } static void @@ -959,8 +961,7 @@ signal_handler(gpointer user_data) int signo = GPOINTER_TO_INT(user_data); _LOG_X_I("Caught signal %d, shutting down...", signo); - g_main_loop_quit(gl.loop); - + gl.shutdown_quitting = TRUE; return G_SOURCE_CONTINUE; } @@ -1014,8 +1015,6 @@ main(int argc, char **argv) } else logging_setup(); - gl.loop = g_main_loop_new(NULL, FALSE); - gl.dbus_connection = g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, &error); if (!gl.dbus_connection) { _LOG_X_W("Could not get the system bus (%s). Make sure the message bus daemon is running!", @@ -1050,7 +1049,11 @@ main(int argc, char **argv) quit_timeout_reschedule(); - g_main_loop_run(gl.loop); + while (TRUE) { + if (gl.shutdown_timeout || gl.shutdown_quitting) + break; + g_main_context_iteration(NULL, TRUE); + } done: @@ -1083,7 +1086,6 @@ done: nm_clear_g_source(&signal_id_term); nm_clear_g_source(&signal_id_int); nm_clear_g_source(&gl.quit_id); - nm_clear_pointer(&gl.loop, g_main_loop_unref); g_clear_object(&gl.dbus_connection); if (!gl.debug) From 33b643414f1e9873d7bf4d6163f677cfdf1aa85b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 09:06:56 +0200 Subject: [PATCH 20/28] dispatcher: use GSource instead of source ids --- src/nm-dispatcher/nm-dispatcher.c | 45 ++++++++++++++++--------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index 923e06b957..ad566c0db7 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -28,7 +28,7 @@ static struct { GDBusConnection *dbus_connection; gboolean debug; gboolean persist; - guint quit_id; + GSource * quit_source; guint request_id_counter; gboolean ever_acquired_name; bool exit_with_failure; @@ -50,8 +50,8 @@ typedef struct { char * error; gboolean wait; gboolean dispatched; - guint watch_id; - guint timeout_id; + GSource * watch_source; + GSource * timeout_source; } ScriptInfo; struct Request { @@ -191,17 +191,17 @@ request_free(Request *request) static gboolean quit_timeout_cb(gpointer user_data) { - gl.quit_id = 0; + nm_clear_g_source_inst(&gl.quit_source); gl.shutdown_timeout = TRUE; - return G_SOURCE_REMOVE; + return G_SOURCE_CONTINUE; } static void quit_timeout_reschedule(void) { if (!gl.persist) { - nm_clear_g_source(&gl.quit_id); - gl.quit_id = g_timeout_add_seconds(10, quit_timeout_cb, NULL); + nm_clear_g_source_inst(&gl.quit_source); + gl.quit_source = nm_g_timeout_add_source_seconds(10, quit_timeout_cb, NULL); } } @@ -367,8 +367,8 @@ script_watch_cb(GPid pid, int status, gpointer user_data) g_assert(pid == script->pid); - script->watch_id = 0; - nm_clear_g_source(&script->timeout_id); + nm_clear_g_source_inst(&script->watch_source); + nm_clear_g_source_inst(&script->timeout_source); script->request->num_scripts_done++; if (!script->wait) script->request->num_scripts_nowait--; @@ -397,8 +397,8 @@ script_timeout_cb(gpointer user_data) { ScriptInfo *script = user_data; - script->timeout_id = 0; - nm_clear_g_source(&script->watch_id); + nm_clear_g_source_inst(&script->timeout_source); + nm_clear_g_source_inst(&script->watch_source); script->request->num_scripts_done++; if (!script->wait) script->request->num_scripts_nowait--; @@ -419,7 +419,7 @@ again: complete_script(script); - return FALSE; + return G_SOURCE_CONTINUE; } static gboolean @@ -516,8 +516,9 @@ script_dispatch(ScriptInfo *script) return FALSE; } - script->watch_id = g_child_watch_add(script->pid, (GChildWatchFunc) script_watch_cb, script); - script->timeout_id = g_timeout_add_seconds(SCRIPT_TIMEOUT, script_timeout_cb, script); + script->watch_source = nm_g_child_watch_add_source(script->pid, script_watch_cb, script); + script->timeout_source = + nm_g_timeout_add_source_seconds(SCRIPT_TIMEOUT, script_timeout_cb, script); if (!script->wait) request->num_scripts_nowait++; return TRUE; @@ -784,7 +785,7 @@ _method_call_action(GDBusMethodInvocation *invocation, GVariant *parameters) return; } - nm_clear_g_source(&gl.quit_id); + nm_clear_g_source_inst(&gl.quit_source); gl.num_requests_pending++; @@ -990,8 +991,8 @@ int main(int argc, char **argv) { gs_free_error GError *error = NULL; - guint signal_id_term = 0; - guint signal_id_int = 0; + GSource * source_term = NULL; + GSource * source_int = NULL; guint dbus_regist_id = 0; guint dbus_own_name_id = 0; @@ -1001,8 +1002,8 @@ main(int argc, char **argv) goto done; } - signal_id_term = g_unix_signal_add(SIGTERM, signal_handler, GINT_TO_POINTER(SIGTERM)); - signal_id_int = g_unix_signal_add(SIGINT, signal_handler, GINT_TO_POINTER(SIGINT)); + source_term = nm_g_unix_signal_add_source(SIGTERM, signal_handler, GINT_TO_POINTER(SIGTERM)); + source_int = nm_g_unix_signal_add_source(SIGINT, signal_handler, GINT_TO_POINTER(SIGINT)); if (gl.debug) { if (!g_getenv("G_MESSAGES_DEBUG")) { @@ -1083,9 +1084,9 @@ done: nm_clear_pointer(&gl.requests_waiting, g_queue_free); - nm_clear_g_source(&signal_id_term); - nm_clear_g_source(&signal_id_int); - nm_clear_g_source(&gl.quit_id); + nm_clear_g_source_inst(&source_term); + nm_clear_g_source_inst(&source_int); + nm_clear_g_source_inst(&gl.quit_source); g_clear_object(&gl.dbus_connection); if (!gl.debug) From e21db61b6de1e81a83fcbfd489294e81c3cf3dc8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 09:30:47 +0200 Subject: [PATCH 21/28] dispatcher: setup signal handler as first The very first and the very last thing we want to do is register (unregister) the signal handler. --- src/nm-dispatcher/nm-dispatcher.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index ad566c0db7..17e8a5d1c8 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -996,15 +996,15 @@ main(int argc, char **argv) guint dbus_regist_id = 0; guint dbus_own_name_id = 0; + source_term = nm_g_unix_signal_add_source(SIGTERM, signal_handler, GINT_TO_POINTER(SIGTERM)); + source_int = nm_g_unix_signal_add_source(SIGINT, signal_handler, GINT_TO_POINTER(SIGINT)); + if (!parse_command_line(&argc, &argv, &error)) { _LOG_X_W("Error parsing command line arguments: %s", error->message); gl.exit_with_failure = TRUE; goto done; } - source_term = nm_g_unix_signal_add_source(SIGTERM, signal_handler, GINT_TO_POINTER(SIGTERM)); - source_int = nm_g_unix_signal_add_source(SIGINT, signal_handler, GINT_TO_POINTER(SIGINT)); - if (gl.debug) { if (!g_getenv("G_MESSAGES_DEBUG")) { /* we log our regular messages using g_debug() and g_info(). @@ -1084,13 +1084,14 @@ done: nm_clear_pointer(&gl.requests_waiting, g_queue_free); - nm_clear_g_source_inst(&source_term); - nm_clear_g_source_inst(&source_int); nm_clear_g_source_inst(&gl.quit_source); g_clear_object(&gl.dbus_connection); if (!gl.debug) logging_shutdown(); + nm_clear_g_source_inst(&source_term); + nm_clear_g_source_inst(&source_int); + return gl.exit_with_failure ? 1 : 0; } From 4dd517ca611bf37c622d79a398f0d95ee02ea539 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 09:58:43 +0200 Subject: [PATCH 22/28] dispatcher: ignore SIGPIPE --- src/nm-dispatcher/nm-dispatcher.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index 17e8a5d1c8..843820f6f1 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -996,6 +996,7 @@ main(int argc, char **argv) guint dbus_regist_id = 0; guint dbus_own_name_id = 0; + signal(SIGPIPE, SIG_IGN); source_term = nm_g_unix_signal_add_source(SIGTERM, signal_handler, GINT_TO_POINTER(SIGTERM)); source_int = nm_g_unix_signal_add_source(SIGINT, signal_handler, GINT_TO_POINTER(SIGINT)); From 442428dbbf3df5d372b562344680dedc5a555b86 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 09:59:04 +0200 Subject: [PATCH 23/28] dispatcher: add cancellable for tracking SIGTERM --- src/nm-dispatcher/nm-dispatcher.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index 843820f6f1..e160d6c94f 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -26,6 +26,7 @@ typedef struct Request Request; static struct { GDBusConnection *dbus_connection; + GCancellable * quit_cancellable; gboolean debug; gboolean persist; GSource * quit_source; @@ -963,6 +964,7 @@ signal_handler(gpointer user_data) _LOG_X_I("Caught signal %d, shutting down...", signo); gl.shutdown_quitting = TRUE; + g_cancellable_cancel(gl.quit_cancellable); return G_SOURCE_CONTINUE; } @@ -1000,6 +1002,8 @@ main(int argc, char **argv) source_term = nm_g_unix_signal_add_source(SIGTERM, signal_handler, GINT_TO_POINTER(SIGTERM)); source_int = nm_g_unix_signal_add_source(SIGINT, signal_handler, GINT_TO_POINTER(SIGINT)); + gl.quit_cancellable = g_cancellable_new(); + if (!parse_command_line(&argc, &argv, &error)) { _LOG_X_W("Error parsing command line arguments: %s", error->message); gl.exit_with_failure = TRUE; @@ -1094,5 +1098,7 @@ done: nm_clear_g_source_inst(&source_term); nm_clear_g_source_inst(&source_int); + g_clear_object(&gl.quit_cancellable); + return gl.exit_with_failure ? 1 : 0; } From 273491922e632d3568a11026e2d93e1725676212 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 10:12:09 +0200 Subject: [PATCH 24/28] dispatcher: use nm_g_bus_get_blocking() to create GDBusConnection The difference is that nm_g_bus_get_blocking() iterates the GMainContext of the caller, and thus it can process and handle SIGTERM signals. Calling g_bus_get_sync() does not iterate the context, and we cannot handle or detect early cancellation. --- src/nm-dispatcher/nm-dispatcher.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index e160d6c94f..039815d313 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -18,6 +18,7 @@ #include #include "libnm-core-aux-extern/nm-dispatcher-api.h" +#include "libnm-glib-aux/nm-dbus-aux.h" #include "nm-dispatcher-utils.h" /*****************************************************************************/ @@ -1021,14 +1022,21 @@ main(int argc, char **argv) } else logging_setup(); - gl.dbus_connection = g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, &error); + gl.dbus_connection = nm_g_bus_get_blocking(gl.quit_cancellable, &error); if (!gl.dbus_connection) { - _LOG_X_W("Could not get the system bus (%s). Make sure the message bus daemon is running!", - error->message); - gl.exit_with_failure = TRUE; + if (!nm_utils_error_is_cancelled(error)) { + _LOG_X_W("dbus: failure to get D-Bus connection: %s", error->message); + gl.exit_with_failure = TRUE; + } goto done; } + /* On bus-disconnect, GDBus will raise(SIGTERM), which we handle like a + * regular request to quit. */ + g_dbus_connection_set_exit_on_close(gl.dbus_connection, TRUE); + + _LOG_X_D("dbus: unique name: %s", g_dbus_connection_get_unique_name(gl.dbus_connection)); + gl.requests_waiting = g_queue_new(); dbus_regist_id = From d127b1fb79c2693f7c62aa964afb74a74e31925b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 11:23:00 +0200 Subject: [PATCH 25/28] dispatcher: minor various cleanup of timeout and shutdown - use nm_g_timeout_add_source() for millisecond precision of idle timeout. - schedule the first idle timeout before registering the D-Bus object. - let the signal handler do nothing, if we are already quitting. In practice, this only silences the extra logging. --- src/nm-dispatcher/nm-dispatcher.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index 039815d313..4ee7153afb 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -201,10 +201,14 @@ quit_timeout_cb(gpointer user_data) static void quit_timeout_reschedule(void) { - if (!gl.persist) { - nm_clear_g_source_inst(&gl.quit_source); - gl.quit_source = nm_g_timeout_add_source_seconds(10, quit_timeout_cb, NULL); - } + if (gl.persist) + return; + + if (gl.shutdown_quitting) + return; + + nm_clear_g_source_inst(&gl.quit_source); + gl.quit_source = nm_g_timeout_add_source(10000, quit_timeout_cb, NULL); } /** @@ -961,11 +965,11 @@ logging_shutdown(void) static gboolean signal_handler(gpointer user_data) { - int signo = GPOINTER_TO_INT(user_data); - - _LOG_X_I("Caught signal %d, shutting down...", signo); - gl.shutdown_quitting = TRUE; - g_cancellable_cancel(gl.quit_cancellable); + if (!gl.shutdown_quitting) { + gl.shutdown_quitting = TRUE; + _LOG_X_I("Caught signal %d, shutting down...", GPOINTER_TO_INT(user_data)); + g_cancellable_cancel(gl.quit_cancellable); + } return G_SOURCE_CONTINUE; } @@ -1039,6 +1043,8 @@ main(int argc, char **argv) gl.requests_waiting = g_queue_new(); + quit_timeout_reschedule(); + dbus_regist_id = g_dbus_connection_register_object(gl.dbus_connection, NM_DISPATCHER_DBUS_PATH, @@ -1061,8 +1067,6 @@ main(int argc, char **argv) NULL, NULL); - quit_timeout_reschedule(); - while (TRUE) { if (gl.shutdown_timeout || gl.shutdown_quitting) break; @@ -1071,6 +1075,9 @@ main(int argc, char **argv) done: + gl.shutdown_quitting = TRUE; + g_cancellable_cancel(gl.quit_cancellable); + /* FIXME: nm-dispatcher does not exit-on-idle in a racefree manner. * See https://lists.freedesktop.org/archives/dbus/2015-May/016671.html */ From 4fe20e4cbeeead68ff9d7480242b2309aff6662b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 12:58:29 +0200 Subject: [PATCH 26/28] dispatcher: fix race for exit-on-idle - exit-on-idle needs to be done correctly. Fix the race, by first notifying systemd (STOPPING=1), releasing the name, and all the while continue processing requests. - don't use g_bus_own_name_on_connection(). That one also listens to NameLost and NameAcquired signals, but we don't care about those. systemd will take care to only spawn one process at a time. And anyway, the well-known name is only important to be reachable, we don't require it to be functional. We can get the first request before RequestName completed and we can continue getting requests after releasing the name. --- data/NetworkManager-dispatcher.service.in | 1 + src/nm-dispatcher/nm-dispatcher.c | 238 ++++++++++++++-------- 2 files changed, 156 insertions(+), 83 deletions(-) diff --git a/data/NetworkManager-dispatcher.service.in b/data/NetworkManager-dispatcher.service.in index c450478bac..b192d0b0c4 100644 --- a/data/NetworkManager-dispatcher.service.in +++ b/data/NetworkManager-dispatcher.service.in @@ -5,6 +5,7 @@ Description=Network Manager Script Dispatcher Service Type=dbus BusName=org.freedesktop.nm_dispatcher ExecStart=@libexecdir@/nm-dispatcher +NotifyAccess=main # We want to allow scripts to spawn long-running daemons, so tell # systemd to not clean up when nm-dispatcher exits diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index 4ee7153afb..c76b9c368f 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -19,6 +19,7 @@ #include "libnm-core-aux-extern/nm-dispatcher-api.h" #include "libnm-glib-aux/nm-dbus-aux.h" +#include "libnm-glib-aux/nm-io-utils.h" #include "nm-dispatcher-utils.h" /*****************************************************************************/ @@ -32,8 +33,11 @@ static struct { gboolean persist; GSource * quit_source; guint request_id_counter; - gboolean ever_acquired_name; - bool exit_with_failure; + guint dbus_regist_id; + + bool name_requested; + + bool exit_with_failure; bool shutdown_timeout; bool shutdown_quitting; @@ -201,13 +205,17 @@ quit_timeout_cb(gpointer user_data) static void quit_timeout_reschedule(void) { + nm_clear_g_source_inst(&gl.quit_source); + if (gl.persist) return; if (gl.shutdown_quitting) return; - nm_clear_g_source_inst(&gl.quit_source); + if (gl.num_requests_pending > 0) + return; + gl.quit_source = nm_g_timeout_add_source(10000, quit_timeout_cb, NULL); } @@ -294,7 +302,7 @@ complete_request(Request *request) request_free(request); - g_assert_cmpuint(gl.num_requests_pending, >, 0); + nm_assert(gl.num_requests_pending > 0); if (--gl.num_requests_pending <= 0) { nm_assert(!gl.current_request && !g_queue_peek_head(gl.requests_waiting)); quit_timeout_reschedule(); @@ -791,9 +799,9 @@ _method_call_action(GDBusMethodInvocation *invocation, GVariant *parameters) return; } - nm_clear_g_source_inst(&gl.quit_source); - gl.num_requests_pending++; + gl.shutdown_timeout = FALSE; + nm_clear_g_source_inst(&gl.quit_source); for (i = 0; i < request->scripts->len; i++) { ScriptInfo *s = g_ptr_array_index(request->scripts, i); @@ -838,31 +846,6 @@ _method_call_action(GDBusMethodInvocation *invocation, GVariant *parameters) } } -static void -on_name_acquired(GDBusConnection *connection, const char *name, gpointer user_data) -{ - gl.ever_acquired_name = TRUE; -} - -static void -on_name_lost(GDBusConnection *connection, const char *name, gpointer user_data) -{ - if (!connection) { - if (!gl.ever_acquired_name) { - _LOG_X_W("Could not get the system bus. Make sure the message bus daemon is running!"); - gl.exit_with_failure = TRUE; - } else { - _LOG_X_I("System bus stopped. Exiting"); - } - } else if (!gl.ever_acquired_name) { - _LOG_X_W("Could not acquire the " NM_DISPATCHER_DBUS_SERVICE " service."); - gl.exit_with_failure = TRUE; - } else - _LOG_X_I("Lost the " NM_DISPATCHER_DBUS_SERVICE " name. Exiting"); - - gl.shutdown_quitting = TRUE; -} - static void _method_call(GDBusConnection * connection, const char * sender, @@ -910,9 +893,75 @@ static GDBusInterfaceInfo *const interface_info = NM_DEFINE_GDBUS_INTERFACE_INFO .out_args = NM_DEFINE_GDBUS_ARG_INFOS(NM_DEFINE_GDBUS_ARG_INFO("results", "a(sus)"), ), ), ), ); -static const GDBusInterfaceVTable interface_vtable = { - .method_call = _method_call, -}; +static gboolean +_bus_register_service(void) +{ + static const GDBusInterfaceVTable interface_vtable = { + .method_call = _method_call, + }; + gs_free_error GError * error = NULL; + NMDBusConnectionCallBlockingData data = { + .result = NULL, + }; + gs_unref_variant GVariant *ret = NULL; + guint32 ret_val; + + gl.dbus_regist_id = + g_dbus_connection_register_object(gl.dbus_connection, + NM_DISPATCHER_DBUS_PATH, + interface_info, + NM_UNCONST_PTR(GDBusInterfaceVTable, &interface_vtable), + NULL, + NULL, + &error); + if (gl.dbus_regist_id == 0) { + _LOG_X_W("dbus: could not export dispatcher D-Bus interface %s: %s", + NM_DISPATCHER_DBUS_PATH, + error->message); + return FALSE; + } + + _LOG_X_D("dbus: dispatcher D-Bus interface %s registered", NM_DISPATCHER_DBUS_PATH); + + gl.name_requested = TRUE; + + nm_dbus_connection_call_request_name(gl.dbus_connection, + NM_DISPATCHER_DBUS_SERVICE, + DBUS_NAME_FLAG_ALLOW_REPLACEMENT + | DBUS_NAME_FLAG_REPLACE_EXISTING, + 10000, + gl.quit_cancellable, + nm_dbus_connection_call_blocking_callback, + &data); + + /* Note that with D-Bus activation, the first request will already hit us before RequestName + * completes. So when we start iterating the main context, the first request may already come + * in. */ + + ret = nm_dbus_connection_call_blocking(&data, &error); + + if (nm_utils_error_is_cancelled(error)) + return FALSE; + + if (error) { + _LOG_X_W("d-bus: failed to request name %s: %s", + NM_DISPATCHER_DBUS_SERVICE, + error->message); + return FALSE; + } + + g_variant_get(ret, "(u)", &ret_val); + + if (ret_val != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) { + _LOG_X_W("dbus: request name for %s failed to take name (response %u)", + NM_DISPATCHER_DBUS_SERVICE, + ret_val); + return FALSE; + } + + _LOG_X_D("dbus: request name for %s succeeded", NM_DISPATCHER_DBUS_SERVICE); + return TRUE; +} /*****************************************************************************/ @@ -973,6 +1022,14 @@ signal_handler(gpointer user_data) return G_SOURCE_CONTINUE; } +static void +_bus_release_name_cb(GObject *source, GAsyncResult *result, gpointer user_data) +{ + nm_assert(gl.num_requests_pending > 0); + gl.num_requests_pending--; + g_main_context_wakeup(NULL); +} + static gboolean parse_command_line(int *p_argc, char ***p_argv, GError **error) { @@ -997,11 +1054,9 @@ parse_command_line(int *p_argc, char ***p_argv, GError **error) int main(int argc, char **argv) { - gs_free_error GError *error = NULL; - GSource * source_term = NULL; - GSource * source_int = NULL; - guint dbus_regist_id = 0; - guint dbus_own_name_id = 0; + gs_free_error GError *error = NULL; + GSource * source_term = NULL; + GSource * source_int = NULL; signal(SIGPIPE, SIG_IGN); source_term = nm_g_unix_signal_add_source(SIGTERM, signal_handler, GINT_TO_POINTER(SIGTERM)); @@ -1045,67 +1100,84 @@ main(int argc, char **argv) quit_timeout_reschedule(); - dbus_regist_id = - g_dbus_connection_register_object(gl.dbus_connection, - NM_DISPATCHER_DBUS_PATH, - interface_info, - NM_UNCONST_PTR(GDBusInterfaceVTable, &interface_vtable), - NULL, - NULL, - &error); - if (dbus_regist_id == 0) { - _LOG_X_W("Could not export Dispatcher D-Bus interface: %s", error->message); - gl.exit_with_failure = 1; - goto done; + if (!_bus_register_service()) { + /* we failed to start the D-Bus service, and will shut down. However, + * first see whether there are any requests that we should process. + * Even if RequestName fails, we might already have requests pending. */ + if (!g_cancellable_is_cancelled(gl.quit_cancellable)) + gl.exit_with_failure = TRUE; + gl.shutdown_quitting = TRUE; } - dbus_own_name_id = g_bus_own_name_on_connection(gl.dbus_connection, - NM_DISPATCHER_DBUS_SERVICE, - G_BUS_NAME_OWNER_FLAGS_NONE, - on_name_acquired, - on_name_lost, - NULL, - NULL); - while (TRUE) { - if (gl.shutdown_timeout || gl.shutdown_quitting) + if (gl.num_requests_pending > 0) { + /* while we have requests pending, we cannot stop processing them... */ + } else if (gl.shutdown_timeout || gl.shutdown_quitting) { + if (gl.name_requested) { + int r; + + /* We already requested a name. To exit-on-idle without race, we need to dance. + * See https://lists.freedesktop.org/archives/dbus/2015-May/016671.html . */ + + gl.name_requested = FALSE; + gl.shutdown_quitting = TRUE; + + _LOG_X_T("shutdown: release-name"); + + /* we create a fake pending request. */ + gl.num_requests_pending++; + nm_clear_g_source_inst(&gl.quit_source); + + r = nm_sd_notify("STOPPING=1"); + if (r < 0) + _LOG_X_W("shutdown: sd_notifiy(STOPPING=1) failed: %s", nm_strerror_native(-r)); + else + _LOG_X_T("shutdown: sd_notifiy(STOPPING=1) succeeded"); + + g_dbus_connection_call(gl.dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "ReleaseName", + g_variant_new("(s)", NM_DISPATCHER_DBUS_SERVICE), + G_VARIANT_TYPE("(u)"), + G_DBUS_CALL_FLAGS_NONE, + 10000, + NULL, + _bus_release_name_cb, + NULL); + continue; + } + break; + } + g_main_context_iteration(NULL, TRUE); } done: + nm_g_main_context_iterate_ready(NULL); gl.shutdown_quitting = TRUE; g_cancellable_cancel(gl.quit_cancellable); - /* FIXME: nm-dispatcher does not exit-on-idle in a racefree manner. - * See https://lists.freedesktop.org/archives/dbus/2015-May/016671.html */ + nm_assert(gl.num_requests_pending == 0); - if (gl.num_requests_pending > 0) { - /* this only happens when we quit due to SIGTERM (not due to the idle timer). - * - * Log a warning about pending scripts. - * - * Maybe we should notify NetworkManager that these scripts are left in an unknown state. - * But this is either a bug of a dispatcher script (not terminating in time). - * - * FIXME(shutdown): Also, currently NetworkManager behaves wrongly on shutdown. - * Note that systemd would not terminate NetworkManager-dispatcher before NetworkManager. - * It's NetworkManager's responsibility to keep running long enough so that all requests - * can complete (with a watchdog timer, and a warning that user provided scripts hang). */ - _LOG_X_W("exiting but there are still %u requests pending", gl.num_requests_pending); - } - - if (dbus_own_name_id != 0) - g_bus_unown_name(nm_steal_int(&dbus_own_name_id)); - - if (dbus_regist_id != 0) - g_dbus_connection_unregister_object(gl.dbus_connection, nm_steal_int(&dbus_regist_id)); + if (gl.dbus_regist_id != 0) + g_dbus_connection_unregister_object(gl.dbus_connection, nm_steal_int(&gl.dbus_regist_id)); nm_clear_pointer(&gl.requests_waiting, g_queue_free); nm_clear_g_source_inst(&gl.quit_source); - g_clear_object(&gl.dbus_connection); + + if (gl.dbus_connection) { + g_dbus_connection_flush_sync(gl.dbus_connection, NULL, NULL); + g_clear_object(&gl.dbus_connection); + } + + nm_g_main_context_iterate_ready(NULL); + + _LOG_X_T("shutdown: exiting with %s", gl.exit_with_failure ? "failure" : "success"); if (!gl.debug) logging_shutdown(); From d25a33f6041168ac0cac728a75854679b8e92b4a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 14:35:24 +0200 Subject: [PATCH 27/28] dispatcher: support enabling debug logging via environment variable The advantage of environment variables is that the user can use `systemctl edit NetworkManager-dispatcher.service` for setting them, without need to change the ExecStart= line. Also, enabling debugging from the start is useful, despite that debug logging can be enabled per-request. Also, there is a difference whether we want verbose logging or whether we want to log to stdout. There should be a flag, that only increases the logging verbosity, but does not change the logging backend. --- data/NetworkManager-dispatcher.service.in | 6 +++ po/POTFILES.skip | 1 + src/nm-dispatcher/nm-dispatcher.c | 53 ++++++++++++++++++----- 3 files changed, 50 insertions(+), 10 deletions(-) diff --git a/data/NetworkManager-dispatcher.service.in b/data/NetworkManager-dispatcher.service.in index b192d0b0c4..1a45f35367 100644 --- a/data/NetworkManager-dispatcher.service.in +++ b/data/NetworkManager-dispatcher.service.in @@ -7,6 +7,12 @@ BusName=org.freedesktop.nm_dispatcher ExecStart=@libexecdir@/nm-dispatcher NotifyAccess=main +# Enable debug logging in dispatcher service. Note that dispatcher +# also honors debug logging requests from NetworkManager, so you +# can also control logging requests with +# `nmcli general logging domain DISPATCHER level TRACE`. +#Environment=NM_DISPATCHER_DEBUG_LOG=1 + # We want to allow scripts to spawn long-running daemons, so tell # systemd to not clean up when nm-dispatcher exits KillMode=process diff --git a/po/POTFILES.skip b/po/POTFILES.skip index 3f70738f8b..399b1e6b5c 100644 --- a/po/POTFILES.skip +++ b/po/POTFILES.skip @@ -1,4 +1,5 @@ contrib/fedora/rpm/ +data/NetworkManager-dispatcher.service.in data/NetworkManager-wait-online.service.in data/NetworkManager.service.in data/nm-sudo.service.in diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index c76b9c368f..8303c6df86 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -24,12 +24,19 @@ /*****************************************************************************/ +/* Serves only the purpose to mark environment variables that are honored by + * the application. You can search for this macro, and find what options are supported. */ +#define _ENV(var) ("" var "") + +/*****************************************************************************/ + typedef struct Request Request; static struct { GDBusConnection *dbus_connection; GCancellable * quit_cancellable; - gboolean debug; + bool log_verbose; + bool log_stdout; gboolean persist; GSource * quit_source; guint request_id_counter; @@ -145,7 +152,7 @@ struct Request { } \ G_STMT_END -#define _LOG_X_D_enabled() (gl.debug) +#define _LOG_X_D_enabled() (gl.log_verbose) #define _LOG_X_T_enabled() _LOG_X_D_enabled() #define _LOG_R_D_enabled(request) (_NM_ENSURE_TYPE_CONST(Request *, request)->debug) @@ -743,7 +750,7 @@ _method_call_action(GDBusMethodInvocation *invocation, GVariant *parameters) request = g_slice_new0(Request); request->request_id = ++gl.request_id_counter; - request->debug = debug || gl.debug; + request->debug = debug || gl.log_verbose; request->context = invocation; request->action = g_strdup(action); @@ -1034,11 +1041,32 @@ static gboolean parse_command_line(int *p_argc, char ***p_argv, GError **error) { GOptionContext *opt_ctx; - GOptionEntry entries[] = { - {"debug", 0, 0, G_OPTION_ARG_NONE, &gl.debug, "Output to console rather than syslog", NULL}, - {"persist", 0, 0, G_OPTION_ARG_NONE, &gl.persist, "Don't quit after a short timeout", NULL}, - {NULL}}; - gboolean success; + gboolean arg_debug = FALSE; + GOptionEntry entries[] = {{ + "debug", + 0, + 0, + G_OPTION_ARG_NONE, + &arg_debug, + "Output to console rather than syslog", + NULL, + }, + { + "persist", + 0, + 0, + G_OPTION_ARG_NONE, + &gl.persist, + "Don't quit after a short timeout", + NULL, + }, + { + NULL, + }}; + gboolean success; + + gl.log_stdout = FALSE; + gl.log_verbose = _nm_utils_ascii_str_to_bool(g_getenv(_ENV("NM_DISPATCHER_DEBUG_LOG")), FALSE); opt_ctx = g_option_context_new(NULL); g_option_context_set_summary(opt_ctx, "Executes scripts upon actions by NetworkManager."); @@ -1048,6 +1076,11 @@ parse_command_line(int *p_argc, char ***p_argv, GError **error) g_option_context_free(opt_ctx); + if (success && arg_debug) { + gl.log_stdout = TRUE; + gl.log_verbose = TRUE; + } + return success; } @@ -1070,7 +1103,7 @@ main(int argc, char **argv) goto done; } - if (gl.debug) { + if (gl.log_stdout) { if (!g_getenv("G_MESSAGES_DEBUG")) { /* we log our regular messages using g_debug() and g_info(). * When we redirect glib logging to syslog, there is no problem. @@ -1179,7 +1212,7 @@ done: _LOG_X_T("shutdown: exiting with %s", gl.exit_with_failure ? "failure" : "success"); - if (!gl.debug) + if (gl.log_stdout) logging_shutdown(); nm_clear_g_source_inst(&source_term); From ff8e85ab5347e42e6f41457054ae80293c9ce142 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Aug 2021 17:58:30 +0200 Subject: [PATCH 28/28] dispatcher: add D-Bus method "Ping" This is only for testing the service. As nm-dispatcher is D-Bus activated, have a simple method to test whether it works. --- src/nm-dispatcher/nm-dispatcher.c | 33 +++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/nm-dispatcher/nm-dispatcher.c b/src/nm-dispatcher/nm-dispatcher.c index 8303c6df86..b96cc82ea9 100644 --- a/src/nm-dispatcher/nm-dispatcher.c +++ b/src/nm-dispatcher/nm-dispatcher.c @@ -20,6 +20,7 @@ #include "libnm-core-aux-extern/nm-dispatcher-api.h" #include "libnm-glib-aux/nm-dbus-aux.h" #include "libnm-glib-aux/nm-io-utils.h" +#include "libnm-glib-aux/nm-time-utils.h" #include "nm-dispatcher-utils.h" /*****************************************************************************/ @@ -42,6 +43,8 @@ static struct { guint request_id_counter; guint dbus_regist_id; + gint64 start_timestamp_msec; + bool name_requested; bool exit_with_failure; @@ -853,6 +856,26 @@ _method_call_action(GDBusMethodInvocation *invocation, GVariant *parameters) } } +static void +_method_call_ping(GDBusMethodInvocation *invocation, GVariant *parameters) +{ + gs_free char *msg = NULL; + gint64 running_msec; + const char * arg_s; + + g_variant_get(parameters, "(&s)", &arg_s); + + running_msec = nm_utils_clock_gettime_msec(CLOCK_BOOTTIME) - gl.start_timestamp_msec; + + msg = g_strdup_printf("pid=%lu, unique-name=%s, since=%" G_GINT64_FORMAT ".%03d, pong=%s", + (unsigned long) getpid(), + g_dbus_connection_get_unique_name(gl.dbus_connection), + (gint64) (running_msec / 1000), + (int) (running_msec % 1000), + arg_s); + g_dbus_method_invocation_return_value(invocation, g_variant_new("(s)", msg)); +} + static void _method_call(GDBusConnection * connection, const char * sender, @@ -868,6 +891,10 @@ _method_call(GDBusConnection * connection, _method_call_action(invocation, parameters); return; } + if (nm_streq(method_name, "Ping")) { + _method_call_ping(invocation, parameters); + return; + } } g_dbus_method_invocation_return_error(invocation, G_DBUS_ERROR, @@ -879,6 +906,10 @@ _method_call(GDBusConnection * connection, static GDBusInterfaceInfo *const interface_info = NM_DEFINE_GDBUS_INTERFACE_INFO( NM_DISPATCHER_DBUS_INTERFACE, .methods = NM_DEFINE_GDBUS_METHOD_INFOS( + NM_DEFINE_GDBUS_METHOD_INFO( + "Ping", + .in_args = NM_DEFINE_GDBUS_ARG_INFOS(NM_DEFINE_GDBUS_ARG_INFO("arg", "s"), ), + .out_args = NM_DEFINE_GDBUS_ARG_INFOS(NM_DEFINE_GDBUS_ARG_INFO("arg", "s"), ), ), NM_DEFINE_GDBUS_METHOD_INFO( "Action", .in_args = NM_DEFINE_GDBUS_ARG_INFOS( @@ -1095,6 +1126,8 @@ main(int argc, char **argv) source_term = nm_g_unix_signal_add_source(SIGTERM, signal_handler, GINT_TO_POINTER(SIGTERM)); source_int = nm_g_unix_signal_add_source(SIGINT, signal_handler, GINT_TO_POINTER(SIGINT)); + gl.start_timestamp_msec = nm_utils_clock_gettime_msec(CLOCK_BOOTTIME); + gl.quit_cancellable = g_cancellable_new(); if (!parse_command_line(&argc, &argv, &error)) {