From 1648fd3c14834ab294054337604e441b2b2754a2 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" (cherry picked from commit d0a6f6f34c705c255d644886b3479e2fb629f41c) --- 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 064a7035dc..6c962799eb 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 f8fc8b3302772f81358abb173a63ec70a1b0eba1 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() (cherry picked from commit fc14d32e99a1f84b37fab0b00418540e29bef452) --- 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 6c962799eb..4626da18ea 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 a61ff3de978727f2aa6d1c6c2edad6f80fa677de 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. (cherry picked from commit a7308bbe9cd9cf2821bea996711fb1b378facad0) --- 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 4626da18ea..9b0f5b559d 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 23bd466af6af1727d11637eab0835cbdbaea85b0 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 (cherry picked from commit 2919b9271d70dd8a21aec326c422088229cdf57b) --- 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 9b0f5b559d..25507a0287 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 eaa068cfb366788fbb822310796dd8fc5cd6ad08 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. (cherry picked from commit 3fa3dba1b1b4a284dbb3e410fd947a4067bccdc7) --- 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 25507a0287..978b459bc3 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 331f571dffef3fee2247e14cfea4fd63de811430 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. (cherry picked from commit 2e3ff56cdc7b1feb2e0fe46aa711f86a07c1bdd2) --- 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 978b459bc3..9d4f684580 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 3f30283a76ff6b81275d1275d4490ea155a3e2f5 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). (cherry picked from commit 753f727af566a56e548613c35a3ccffe3d212d3b) --- 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 9d4f684580..627e6c2026 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 529399125583eb8f876cc52c6c0f00d5523bae57 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(). (cherry picked from commit a09a5f7fc116ba5531c431e73a87b4cd87ace2b9) --- 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 627e6c2026..a006d4c7c1 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); }