From d0a6f6f34c705c255d644886b3479e2fb629f41c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 25 Apr 2016 12:32:12 +0200 Subject: [PATCH 1/8] sleep-monitor: use LOG macros in "nm-sleep-monitor-systemd.c" --- src/nm-sleep-monitor-systemd.c | 48 ++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/src/nm-sleep-monitor-systemd.c b/src/nm-sleep-monitor-systemd.c index 064a7035d..6c962799e 100644 --- a/src/nm-sleep-monitor-systemd.c +++ b/src/nm-sleep-monitor-systemd.c @@ -75,13 +75,42 @@ static guint signals[LAST_SIGNAL] = {0}; G_DEFINE_TYPE (NMSleepMonitor, nm_sleep_monitor, G_TYPE_OBJECT); -/********************************************************************/ +NM_DEFINE_SINGLETON_GETTER (NMSleepMonitor, nm_sleep_monitor_get, NM_TYPE_SLEEP_MONITOR); + +/*****************************************************************************/ + +#ifdef SUSPEND_RESUME_SYSTEMD +#define _NMLOG_PREFIX_NAME "sleep-monitor-sd" +#else +#define _NMLOG_PREFIX_NAME "sleep-monitor-ck" +#endif + +#define _NMLOG_DOMAIN LOGD_SUSPEND +#define _NMLOG(level, ...) \ + G_STMT_START { \ + const NMLogLevel __level = (level); \ + \ + if (nm_logging_enabled (__level, _NMLOG_DOMAIN)) { \ + char __prefix[20]; \ + const NMSleepMonitor *const __self = (self); \ + \ + _nm_log (__level, _NMLOG_DOMAIN, 0, \ + "%s%s: " _NM_UTILS_MACRO_FIRST (__VA_ARGS__), \ + _NMLOG_PREFIX_NAME, \ + (!__self || __self == singleton_instance \ + ? "" \ + : nm_sprintf_buf (__prefix, "[%p]", __self)) \ + _NM_UTILS_MACRO_REST (__VA_ARGS__)); \ + } \ + } G_STMT_END + +/*****************************************************************************/ static gboolean drop_inhibitor (NMSleepMonitor *self) { if (self->inhibit_fd >= 0) { - nm_log_dbg (LOGD_SUSPEND, "Dropping systemd sleep inhibitor"); + _LOGD ("Dropping systemd sleep inhibitor"); close (self->inhibit_fd); self->inhibit_fd = -1; return TRUE; @@ -103,15 +132,15 @@ inhibit_done (GObject *source, res = g_dbus_proxy_call_with_unix_fd_list_finish (sd_proxy, &fd_list, result, &error); if (!res) { g_dbus_error_strip_remote_error (error); - nm_log_warn (LOGD_SUSPEND, "Inhibit failed: %s", error->message); + _LOGW ("Inhibit failed: %s", error->message); g_error_free (error); } else { if (!fd_list || g_unix_fd_list_get_length (fd_list) != 1) - nm_log_warn (LOGD_SUSPEND, "Didn't get a single fd back"); + _LOGW ("Didn't get a single fd back"); self->inhibit_fd = g_unix_fd_list_get (fd_list, 0, NULL); - nm_log_dbg (LOGD_SUSPEND, "Inhibitor fd is %d", self->inhibit_fd); + _LOGD ("Inhibitor fd is %d", self->inhibit_fd); g_object_unref (fd_list); g_variant_unref (res); } @@ -122,7 +151,7 @@ take_inhibitor (NMSleepMonitor *self) { g_assert (self->inhibit_fd == -1); - nm_log_dbg (LOGD_SUSPEND, "Taking systemd sleep inhibitor"); + _LOGD ("Taking systemd sleep inhibitor"); g_dbus_proxy_call_with_unix_fd_list (self->sd_proxy, "Inhibit", g_variant_new ("(ssss)", @@ -145,7 +174,7 @@ prepare_for_sleep_cb (GDBusProxy *proxy, { NMSleepMonitor *self = data; - nm_log_dbg (LOGD_SUSPEND, "Received PrepareForSleep signal: %d", is_about_to_suspend); + _LOGD ("Received PrepareForSleep signal: %d", is_about_to_suspend); if (is_about_to_suspend) { g_signal_emit (self, signals[SLEEPING], 0); @@ -185,7 +214,7 @@ on_proxy_acquired (GObject *object, self->sd_proxy = g_dbus_proxy_new_for_bus_finish (res, &error); if (!self->sd_proxy) { - nm_log_warn (LOGD_SUSPEND, "Failed to acquire logind proxy: %s", error->message); + _LOGW ("Failed to acquire logind proxy: %s", error->message); g_clear_error (&error); return; } @@ -253,6 +282,3 @@ nm_sleep_monitor_class_init (NMSleepMonitorClass *klass) G_TYPE_NONE, 0); } -NM_DEFINE_SINGLETON_GETTER (NMSleepMonitor, nm_sleep_monitor_get, NM_TYPE_SLEEP_MONITOR); - -/* ---------------------------------------------------------------------------------------------------- */ From fc14d32e99a1f84b37fab0b00418540e29bef452 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 25 Apr 2016 12:50:49 +0200 Subject: [PATCH 2/8] sleep-monitor: don't return value from drop_inhibitor() --- src/nm-sleep-monitor-systemd.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/nm-sleep-monitor-systemd.c b/src/nm-sleep-monitor-systemd.c index 6c962799e..4626da18e 100644 --- a/src/nm-sleep-monitor-systemd.c +++ b/src/nm-sleep-monitor-systemd.c @@ -106,16 +106,14 @@ NM_DEFINE_SINGLETON_GETTER (NMSleepMonitor, nm_sleep_monitor_get, NM_TYPE_SLEEP_ /*****************************************************************************/ -static gboolean +static void drop_inhibitor (NMSleepMonitor *self) { if (self->inhibit_fd >= 0) { - _LOGD ("Dropping systemd sleep inhibitor"); + _LOGD ("Dropping systemd sleep inhibitor %d", self->inhibit_fd); close (self->inhibit_fd); self->inhibit_fd = -1; - return TRUE; } - return FALSE; } static void From a7308bbe9cd9cf2821bea996711fb1b378facad0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 25 Apr 2016 12:52:03 +0200 Subject: [PATCH 3/8] sleep-monitor: implement dispose() instead of finalize() To release resources, dispose() is preferred over finalize() because it is called earlier. --- src/nm-sleep-monitor-systemd.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/nm-sleep-monitor-systemd.c b/src/nm-sleep-monitor-systemd.c index 4626da18e..9b0f5b559 100644 --- a/src/nm-sleep-monitor-systemd.c +++ b/src/nm-sleep-monitor-systemd.c @@ -241,16 +241,15 @@ nm_sleep_monitor_init (NMSleepMonitor *self) } static void -finalize (GObject *object) +dispose (GObject *object) { NMSleepMonitor *self = NM_SLEEP_MONITOR (object); drop_inhibitor (self); - if (self->sd_proxy) - g_object_unref (self->sd_proxy); - if (G_OBJECT_CLASS (nm_sleep_monitor_parent_class)->finalize != NULL) - G_OBJECT_CLASS (nm_sleep_monitor_parent_class)->finalize (object); + g_clear_object (&self->sd_proxy); + + G_OBJECT_CLASS (nm_sleep_monitor_parent_class)->dispose (object); } static void @@ -260,7 +259,7 @@ nm_sleep_monitor_class_init (NMSleepMonitorClass *klass) gobject_class = G_OBJECT_CLASS (klass); - gobject_class->finalize = finalize; + gobject_class->dispose = dispose; signals[SLEEPING] = g_signal_new (NM_SLEEP_MONITOR_SLEEPING, NM_TYPE_SLEEP_MONITOR, From 2919b9271d70dd8a21aec326c422088229cdf57b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 25 Apr 2016 12:54:39 +0200 Subject: [PATCH 4/8] sleep-monitor: drop unused class methods for signals --- src/nm-sleep-monitor-systemd.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/nm-sleep-monitor-systemd.c b/src/nm-sleep-monitor-systemd.c index 9b0f5b559..25507a028 100644 --- a/src/nm-sleep-monitor-systemd.c +++ b/src/nm-sleep-monitor-systemd.c @@ -60,12 +60,8 @@ struct _NMSleepMonitor { struct _NMSleepMonitorClass { GObjectClass parent_class; - - void (*sleeping) (NMSleepMonitor *monitor); - void (*resuming) (NMSleepMonitor *monitor); }; - enum { SLEEPING, RESUMING, @@ -264,17 +260,13 @@ nm_sleep_monitor_class_init (NMSleepMonitorClass *klass) signals[SLEEPING] = g_signal_new (NM_SLEEP_MONITOR_SLEEPING, NM_TYPE_SLEEP_MONITOR, G_SIGNAL_RUN_LAST, - G_STRUCT_OFFSET (NMSleepMonitorClass, sleeping), - NULL, /* accumulator */ - NULL, /* accumulator data */ + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); signals[RESUMING] = g_signal_new (NM_SLEEP_MONITOR_RESUMING, NM_TYPE_SLEEP_MONITOR, G_SIGNAL_RUN_LAST, - G_STRUCT_OFFSET (NMSleepMonitorClass, resuming), - NULL, /* accumulator */ - NULL, /* accumulator data */ + 0, NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); } From 3fa3dba1b1b4a284dbb3e410fd947a4067bccdc7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 25 Apr 2016 13:00:37 +0200 Subject: [PATCH 5/8] sleep-monitor: handle early destruction of NMSleepMonitor instance When destroing the sleep monitor before the D-Bus proxy is created, we must cancel creation of the proxy. --- src/nm-sleep-monitor-systemd.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/nm-sleep-monitor-systemd.c b/src/nm-sleep-monitor-systemd.c index 25507a028..978b459bc 100644 --- a/src/nm-sleep-monitor-systemd.c +++ b/src/nm-sleep-monitor-systemd.c @@ -55,6 +55,7 @@ struct _NMSleepMonitor { GObject parent_instance; GDBusProxy *sd_proxy; + GCancellable *cancellable; gint inhibit_fd; }; @@ -205,13 +206,17 @@ on_proxy_acquired (GObject *object, { GError *error = NULL; char *owner; + GDBusProxy *sd_proxy; - self->sd_proxy = g_dbus_proxy_new_for_bus_finish (res, &error); - if (!self->sd_proxy) { - _LOGW ("Failed to acquire logind proxy: %s", error->message); + sd_proxy = g_dbus_proxy_new_for_bus_finish (res, &error); + if (!sd_proxy) { + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + _LOGW ("Failed to acquire logind proxy: %s", error->message); g_clear_error (&error); return; } + self->sd_proxy = sd_proxy; + g_clear_object (&self->cancellable); g_signal_connect (self->sd_proxy, "notify::g-name-owner", G_CALLBACK (name_owner_cb), self); _nm_dbus_signal_connect (self->sd_proxy, "PrepareForSleep", G_VARIANT_TYPE ("(b)"), @@ -227,12 +232,13 @@ static void nm_sleep_monitor_init (NMSleepMonitor *self) { self->inhibit_fd = -1; + self->cancellable = g_cancellable_new (); g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START | G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, NULL, SUSPEND_DBUS_NAME, SUSPEND_DBUS_PATH, SUSPEND_DBUS_INTERFACE, - NULL, + self->cancellable, (GAsyncReadyCallback) on_proxy_acquired, self); } @@ -243,6 +249,8 @@ dispose (GObject *object) drop_inhibitor (self); + nm_clear_g_cancellable (&self->cancellable); + g_clear_object (&self->sd_proxy); G_OBJECT_CLASS (nm_sleep_monitor_parent_class)->dispose (object); From 2e3ff56cdc7b1feb2e0fe46aa711f86a07c1bdd2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 25 Apr 2016 13:27:13 +0200 Subject: [PATCH 6/8] sleep-monitor: properly handle cancelling of "Inhibit" D-Bus call As we don't take a reference on @self during the asynchronous request, we must properly support cancelling in case of early destruction. I think, it's gdbus' responsibility not to leak any file descriptors when cancelling a D-Bus request that returns file descriptors. Thus, our usual pattern works here too. --- src/nm-sleep-monitor-systemd.c | 49 +++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/src/nm-sleep-monitor-systemd.c b/src/nm-sleep-monitor-systemd.c index 978b459bc..9d4f68458 100644 --- a/src/nm-sleep-monitor-systemd.c +++ b/src/nm-sleep-monitor-systemd.c @@ -55,7 +55,10 @@ struct _NMSleepMonitor { GObject parent_instance; GDBusProxy *sd_proxy; + + /* used both during construction of sd_proxy and during Inhibit call. */ GCancellable *cancellable; + gint inhibit_fd; }; @@ -111,6 +114,8 @@ drop_inhibitor (NMSleepMonitor *self) close (self->inhibit_fd); self->inhibit_fd = -1; } + + nm_clear_g_cancellable (&self->cancellable); } static void @@ -120,33 +125,40 @@ inhibit_done (GObject *source, { GDBusProxy *sd_proxy = G_DBUS_PROXY (source); NMSleepMonitor *self = user_data; - GError *error = NULL; - GVariant *res; - GUnixFDList *fd_list; + gs_free_error GError *error = NULL; + gs_unref_variant GVariant *res = NULL; + gs_unref_object GUnixFDList *fd_list = NULL; res = g_dbus_proxy_call_with_unix_fd_list_finish (sd_proxy, &fd_list, result, &error); if (!res) { - g_dbus_error_strip_remote_error (error); - _LOGW ("Inhibit failed: %s", error->message); - g_error_free (error); - } else { - if (!fd_list || g_unix_fd_list_get_length (fd_list) != 1) - _LOGW ("Didn't get a single fd back"); - - self->inhibit_fd = g_unix_fd_list_get (fd_list, 0, NULL); - - _LOGD ("Inhibitor fd is %d", self->inhibit_fd); - g_object_unref (fd_list); - g_variant_unref (res); + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { + g_clear_object (&self->cancellable); + _LOGW ("Inhibit failed: %s", error->message); + } + return; } + + g_clear_object (&self->cancellable); + + if (!fd_list || g_unix_fd_list_get_length (fd_list) != 1) { + _LOGW ("Didn't get a single fd back"); + return; + } + + self->inhibit_fd = g_unix_fd_list_get (fd_list, 0, NULL); + _LOGD ("Inhibitor fd is %d", self->inhibit_fd); } static void take_inhibitor (NMSleepMonitor *self) { - g_assert (self->inhibit_fd == -1); + g_return_if_fail (NM_IS_SLEEP_MONITOR (self)); + g_return_if_fail (G_IS_DBUS_PROXY (self->sd_proxy)); + + drop_inhibitor (self); _LOGD ("Taking systemd sleep inhibitor"); + self->cancellable = g_cancellable_new (); g_dbus_proxy_call_with_unix_fd_list (self->sd_proxy, "Inhibit", g_variant_new ("(ssss)", @@ -157,7 +169,7 @@ take_inhibitor (NMSleepMonitor *self) 0, G_MAXINT, NULL, - NULL, + self->cancellable, inhibit_done, self); } @@ -247,10 +259,9 @@ dispose (GObject *object) { NMSleepMonitor *self = NM_SLEEP_MONITOR (object); + /* drop_inhibitor() also clears our "cancellable" */ drop_inhibitor (self); - nm_clear_g_cancellable (&self->cancellable); - g_clear_object (&self->sd_proxy); G_OBJECT_CLASS (nm_sleep_monitor_parent_class)->dispose (object); From 753f727af566a56e548613c35a3ccffe3d212d3b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 25 Apr 2016 13:36:16 +0200 Subject: [PATCH 7/8] sleep-monitor: don't localize messages in core daemon The daemon does not run with a particular locale of a user. Localizing makes no sense (at least, we don't do it usually and it would make logging localized). --- src/nm-sleep-monitor-systemd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nm-sleep-monitor-systemd.c b/src/nm-sleep-monitor-systemd.c index 9d4f68458..627e6c202 100644 --- a/src/nm-sleep-monitor-systemd.c +++ b/src/nm-sleep-monitor-systemd.c @@ -164,7 +164,7 @@ take_inhibitor (NMSleepMonitor *self) g_variant_new ("(ssss)", "sleep", "NetworkManager", - _("NetworkManager needs to turn off networks"), + "NetworkManager needs to turn off networks", "delay"), 0, G_MAXINT, From a09a5f7fc116ba5531c431e73a87b4cd87ace2b9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 25 Apr 2016 13:50:54 +0200 Subject: [PATCH 8/8] sleep-monitor: disconnect signal handlers from D-Bus proxy on destroy The lifetime of the proxy is not necesarily the same as the lifetime of the NMSleepMonitor instance. Disconnect the signals during dispose(). --- src/nm-sleep-monitor-systemd.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/nm-sleep-monitor-systemd.c b/src/nm-sleep-monitor-systemd.c index 627e6c202..a006d4c7c 100644 --- a/src/nm-sleep-monitor-systemd.c +++ b/src/nm-sleep-monitor-systemd.c @@ -60,6 +60,9 @@ struct _NMSleepMonitor { GCancellable *cancellable; gint inhibit_fd; + + gulong sig_id_1; + gulong sig_id_2; }; struct _NMSleepMonitorClass { @@ -230,9 +233,11 @@ on_proxy_acquired (GObject *object, self->sd_proxy = sd_proxy; g_clear_object (&self->cancellable); - g_signal_connect (self->sd_proxy, "notify::g-name-owner", G_CALLBACK (name_owner_cb), self); - _nm_dbus_signal_connect (self->sd_proxy, "PrepareForSleep", G_VARIANT_TYPE ("(b)"), - G_CALLBACK (prepare_for_sleep_cb), self); + self->sig_id_1 = g_signal_connect (self->sd_proxy, "notify::g-name-owner", + G_CALLBACK (name_owner_cb), self); + self->sig_id_2 = _nm_dbus_signal_connect (self->sd_proxy, "PrepareForSleep", + G_VARIANT_TYPE ("(b)"), + G_CALLBACK (prepare_for_sleep_cb), self); owner = g_dbus_proxy_get_name_owner (self->sd_proxy); if (owner) @@ -262,7 +267,11 @@ dispose (GObject *object) /* drop_inhibitor() also clears our "cancellable" */ drop_inhibitor (self); - g_clear_object (&self->sd_proxy); + if (self->sd_proxy) { + nm_clear_g_signal_handler (self->sd_proxy, &self->sig_id_1); + nm_clear_g_signal_handler (self->sd_proxy, &self->sig_id_2); + g_clear_object (&self->sd_proxy); + } G_OBJECT_CLASS (nm_sleep_monitor_parent_class)->dispose (object); }