From 2770af85ac04fd14af2f6bcdf4d3967ed6f2e36f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 21 Jul 2018 23:07:53 +0900 Subject: [PATCH 1/3] timedate: increment reference count of sd_bus_message The commit 5d280742b645a69a19e7f9131adc0c95f5c7fa07 introduces a barrier to suppress calling context_update_ntp_status() multiple times. However, it just stores the address of sd_bus_message object. So, when an address is reused on the subsequent message, then the status of NTP clients are not updated. This makes the stored message object is referenced by the context object. So, the subsequent message is on cirtainly different address. --- src/timedate/timedated.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/timedate/timedated.c b/src/timedate/timedated.c index 4beded5de0..6c957646ea 100644 --- a/src/timedate/timedated.c +++ b/src/timedate/timedated.c @@ -44,6 +44,7 @@ typedef struct Context { char *zone; bool local_rtc; Hashmap *polkit_registry; + sd_bus_message *cache; LIST_HEAD(UnitStatusInfo, units); } Context; @@ -71,6 +72,7 @@ static void context_free(Context *c) { free(c->zone); bus_verify_polkit_async_registry_free(c->polkit_registry); + sd_bus_message_unref(c->cache); while ((p = c->units)) { LIST_REMOVE(units, c->units, p); @@ -302,18 +304,20 @@ static int context_update_ntp_status(Context *c, sd_bus *bus, sd_bus_message *m) { "UnitFileState", "s", NULL, offsetof(UnitStatusInfo, unit_file_state) }, {} }; - static sd_bus_message *_m = NULL; UnitStatusInfo *u; int r; assert(c); assert(bus); - /* Suppress multiple call of context_update_ntp_status() within single DBus transaction. */ - if (m && m == _m) - return 0; + /* Suppress calling context_update_ntp_status() multiple times within single DBus transaction. */ + if (m) { + if (m == c->cache) + return 0; - _m = m; + sd_bus_message_unref(c->cache); + c->cache = sd_bus_message_ref(m); + } LIST_FOREACH(units, u, c->units) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; From 65d3426677569c0645b54c7124a9ff7980605c94 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 21 Jul 2018 23:21:28 +0900 Subject: [PATCH 2/3] locale: increment reference count of sd_bus_message --- src/locale/keymap-util.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/locale/keymap-util.c b/src/locale/keymap-util.c index 598b931d03..6538da3b6f 100644 --- a/src/locale/keymap-util.c +++ b/src/locale/keymap-util.c @@ -72,6 +72,10 @@ void context_free(Context *c) { context_free_locale(c); context_free_x11(c); context_free_vconsole(c); + + sd_bus_message_unref(c->locale_cache); + sd_bus_message_unref(c->x11_cache); + sd_bus_message_unref(c->vc_cache); }; void locale_simplify(char *locale[_VARIABLE_LC_MAX]) { @@ -87,11 +91,13 @@ int locale_read_data(Context *c, sd_bus_message *m) { int r; /* Do not try to re-read the file within single bus operation. */ - if (m && m == c->locale_cache) - return 0; + if (m) { + if (m == c->locale_cache) + return 0; - /* To suppress multiple call of stat(), store the message to cache here. */ - c->locale_cache = m; + sd_bus_message_unref(c->locale_cache); + c->locale_cache = sd_bus_message_ref(m); + } r = stat("/etc/locale.conf", &st); if (r < 0 && errno != ENOENT) @@ -155,11 +161,13 @@ int vconsole_read_data(Context *c, sd_bus_message *m) { int r; /* Do not try to re-read the file within single bus operation. */ - if (m && m == c->vc_cache) - return 0; + if (m) { + if (m == c->vc_cache) + return 0; - /* To suppress multiple call of stat(), store the message to cache here. */ - c->vc_cache = m; + sd_bus_message_unref(c->vc_cache); + c->vc_cache = sd_bus_message_ref(m); + } if (stat("/etc/vconsole.conf", &st) < 0) { if (errno != ENOENT) @@ -197,11 +205,13 @@ int x11_read_data(Context *c, sd_bus_message *m) { int r; /* Do not try to re-read the file within single bus operation. */ - if (m && m == c->x11_cache) - return 0; + if (m) { + if (m == c->x11_cache) + return 0; - /* To suppress multiple call of stat(), store the message to cache here. */ - c->x11_cache = m; + sd_bus_message_unref(c->x11_cache); + c->x11_cache = sd_bus_message_ref(m); + } if (stat("/etc/X11/xorg.conf.d/00-keyboard.conf", &st) < 0) { if (errno != ENOENT) From 3af0a96c0fcc623bd16649fc3640396a657cf9ef Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 22 Jul 2018 23:10:02 +0900 Subject: [PATCH 3/3] timedate: defer the property changed signal until job of starting/stopping NTP service is finished Before this, the property changed signal is emitted immediately after StartUnit/StopUnit method is called. So, the running state of the NTP client service may not updated. This makes the timing of emitting property changed signal is deferred until job of starting/stopping NTP client service is completed. Fixes #9672. --- src/timedate/timedated.c | 78 +++++++++++++++++++++++++++++++++------- 1 file changed, 65 insertions(+), 13 deletions(-) diff --git a/src/timedate/timedated.c b/src/timedate/timedated.c index 6c957646ea..012cbe0028 100644 --- a/src/timedate/timedated.c +++ b/src/timedate/timedated.c @@ -46,6 +46,9 @@ typedef struct Context { Hashmap *polkit_registry; sd_bus_message *cache; + sd_bus_slot *slot_job_removed; + char *path_ntp_unit; + LIST_HEAD(UnitStatusInfo, units); } Context; @@ -74,6 +77,9 @@ static void context_free(Context *c) { bus_verify_polkit_async_registry_free(c->polkit_registry); sd_bus_message_unref(c->cache); + sd_bus_slot_unref(c->slot_job_removed); + free(c->path_ntp_unit); + while ((p = c->units)) { LIST_REMOVE(units, c->units, p); unit_status_info_free(p); @@ -345,17 +351,55 @@ static int context_update_ntp_status(Context *c, sd_bus *bus, sd_bus_message *m) return 0; } -static int unit_start_or_stop(UnitStatusInfo *u, sd_bus *bus, sd_bus_error *error, bool start) { +static int match_job_removed(sd_bus_message *m, void *userdata, sd_bus_error *error) { + const char *path; + Context *c = userdata; int r; + assert(c); + assert(m); + + r = sd_bus_message_read(m, "uoss", NULL, &path, NULL, NULL); + if (r < 0) { + bus_log_parse_error(r); + return 0; + } + + if (!streq_ptr(path, c->path_ntp_unit)) + return 0; + + (void) sd_bus_emit_properties_changed(sd_bus_message_get_bus(m), "/org/freedesktop/timedate1", "org.freedesktop.timedate1", "NTP", NULL); + + c->slot_job_removed = sd_bus_slot_unref(c->slot_job_removed); + c->path_ntp_unit = mfree(c->path_ntp_unit); + + return 0; +} + +static int unit_start_or_stop(Context *c, UnitStatusInfo *u, sd_bus *bus, sd_bus_error *error, bool start) { + _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; + _cleanup_(sd_bus_slot_unrefp) sd_bus_slot *slot = NULL; + const char *path; + int r; + + assert(c); assert(u); assert(bus); assert(error); - /* Call context_update_ntp_status() to update UnitStatusInfo before calling this. */ + /* This method may be called frequently. Forget the previous job if it has not completed yet. */ + c->slot_job_removed = sd_bus_slot_unref(c->slot_job_removed); - if (streq(u->active_state, "active") == start) - return 0; + r = sd_bus_match_signal_async( + bus, + &slot, + "org.freedesktop.systemd1", + "/org/freedesktop/systemd1", + "org.freedesktop.systemd1.Manager", + "JobRemoved", + match_job_removed, NULL, c); + if (r < 0) + return r; r = sd_bus_call_method( bus, @@ -364,13 +408,22 @@ static int unit_start_or_stop(UnitStatusInfo *u, sd_bus *bus, sd_bus_error *erro "org.freedesktop.systemd1.Manager", start ? "StartUnit" : "StopUnit", error, - NULL, + &reply, "ss", u->name, "replace"); if (r < 0) return r; + r = sd_bus_message_read(reply, "o", &path); + if (r < 0) + return bus_log_parse_error(r); + + r = free_and_strdup(&c->path_ntp_unit, path); + if (r < 0) + return log_oom(); + + c->slot_job_removed = TAKE_PTR(slot); return 0; } @@ -422,8 +475,9 @@ static int unit_enable_or_disable(UnitStatusInfo *u, sd_bus *bus, sd_bus_error * error, NULL, NULL); - if (r < 0) - return r; + if (r < 0) + return r; + return 0; } @@ -813,7 +867,7 @@ static int method_set_ntp(sd_bus_message *m, void *userdata, sd_bus_error *error if (q < 0) r = q; - q = unit_start_or_stop(u, bus, error, enable); + q = unit_start_or_stop(c, u, bus, error, enable); if (q < 0) r = q; } @@ -827,17 +881,17 @@ static int method_set_ntp(sd_bus_message *m, void *userdata, sd_bus_error *error if (r < 0) continue; - r = unit_start_or_stop(u, bus, error, enable); + r = unit_start_or_stop(c, u, bus, error, enable); break; } - else if (context_ntp_service_is_active(c) <= 0) + else LIST_FOREACH(units, u, c->units) { if (!streq(u->load_state, "loaded") || !streq(u->unit_file_state, "enabled")) continue; - r = unit_start_or_stop(u, bus, error, enable); + r = unit_start_or_stop(c, u, bus, error, enable); break; } @@ -846,8 +900,6 @@ static int method_set_ntp(sd_bus_message *m, void *userdata, sd_bus_error *error log_info("Set NTP to %sd", enable_disable(enable)); - (void) sd_bus_emit_properties_changed(bus, "/org/freedesktop/timedate1", "org.freedesktop.timedate1", "NTP", NULL); - return sd_bus_reply_method_return(m, NULL); }