From 37d0b962eff19234d0b485c14b007081d2067ab9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 27 Nov 2018 20:09:10 +0100 Subject: [PATCH 01/11] core: when we manage to resolve a user, only enqueue dbus event, don't send out message right-away Let's only enqueue the dbus signal generation, let's not do it right-away, after all we want coalescing to take effect here. --- src/core/unit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/unit.c b/src/core/unit.c index 89bb95e2f1..cc4ae50516 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4928,7 +4928,7 @@ void unit_notify_user_lookup(Unit *u, uid_t uid, gid_t gid) { r = unit_ref_uid_gid(u, uid, gid); if (r > 0) - bus_unit_send_change_signal(u); + unit_add_to_dbus_queue(u); } int unit_set_invocation_id(Unit *u, sd_id128_t id) { From 6fcbec6f9b15e534badd069610e35f4c5303c502 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 27 Nov 2018 20:15:45 +0100 Subject: [PATCH 02/11] core: whenever we change state of a unit, force out PropertiesChanged bus signal This allows clients to follow our internal state changes safely. Previously, quick state changes (for example, when we restart a unit due to Restart= after it quickly transitioned through DEAD/FAILED states) would be coalesced into one bus signal event, with this change there's the guarantee that all state changes after the unit was announced ones are reflected on th bus. Note we only do this kind of guaranteed flushing only for unit state changes, not for other unit property changes, where clients still have to expect coalescing. This is because the unit state is a very important, high-level concept. Fixes: #10185 --- src/core/automount.c | 4 ++++ src/core/dbus-unit.c | 21 +++++++++++++++++++++ src/core/dbus-unit.h | 1 + src/core/device.c | 4 ++++ src/core/mount.c | 4 ++++ src/core/path.c | 4 ++++ src/core/scope.c | 4 ++++ src/core/service.c | 4 ++++ src/core/slice.c | 4 ++++ src/core/socket.c | 4 ++++ src/core/swap.c | 4 ++++ src/core/target.c | 4 ++++ src/core/timer.c | 4 ++++ 13 files changed, 66 insertions(+) diff --git a/src/core/automount.c b/src/core/automount.c index 3d8348e0b7..de8010bf2e 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -16,6 +16,7 @@ #include "bus-error.h" #include "bus-util.h" #include "dbus-automount.h" +#include "dbus-unit.h" #include "fd-util.h" #include "format-util.h" #include "io-util.h" @@ -237,6 +238,9 @@ static void automount_set_state(Automount *a, AutomountState state) { AutomountState old_state; assert(a); + if (a->state != state) + bus_unit_send_pending_change_signal(UNIT(a), false); + old_state = a->state; a->state = state; diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 6d9b559d2c..4cf7364c50 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -1202,6 +1202,27 @@ void bus_unit_send_change_signal(Unit *u) { u->sent_dbus_new_signal = true; } +void bus_unit_send_pending_change_signal(Unit *u, bool including_new) { + + /* Sends out any pending change signals, but only if they really are pending. This call is used when we are + * about to change state in order to force out a PropertiesChanged signal beforehand if there was one pending + * so that clients can follow the full state transition */ + + if (!u->in_dbus_queue) /* If not enqueued, don't bother */ + return; + + if (!u->sent_dbus_new_signal && !including_new) /* If the unit was never announced, don't bother, it's fine if + * the unit appears in the new state right-away (except if the + * caller explicitly asked us to send it anyway) */ + return; + + if (MANAGER_IS_RELOADING(u->manager)) /* Don't generate unnecessary PropertiesChanged signals for the same unit + * when we are reloading. */ + return; + + bus_unit_send_change_signal(u); +} + static int send_removed_signal(sd_bus *bus, void *userdata) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; _cleanup_free_ char *p = NULL; diff --git a/src/core/dbus-unit.h b/src/core/dbus-unit.h index 68eb621836..345345e3eb 100644 --- a/src/core/dbus-unit.h +++ b/src/core/dbus-unit.h @@ -11,6 +11,7 @@ extern const sd_bus_vtable bus_unit_vtable[]; extern const sd_bus_vtable bus_unit_cgroup_vtable[]; void bus_unit_send_change_signal(Unit *u); +void bus_unit_send_pending_change_signal(Unit *u, bool including_new); void bus_unit_send_removed_signal(Unit *u); int bus_unit_method_start_generic(sd_bus_message *message, Unit *u, JobType job_type, bool reload_if_possible, sd_bus_error *error); diff --git a/src/core/device.c b/src/core/device.c index 8b6126c4cf..5acd9b7a70 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -6,6 +6,7 @@ #include "alloc-util.h" #include "bus-error.h" #include "dbus-device.h" +#include "dbus-unit.h" #include "device-private.h" #include "device-util.h" #include "device.h" @@ -115,6 +116,9 @@ static void device_set_state(Device *d, DeviceState state) { DeviceState old_state; assert(d); + if (d->state != state) + bus_unit_send_pending_change_signal(UNIT(d), false); + old_state = d->state; d->state = state; diff --git a/src/core/mount.c b/src/core/mount.c index 99b2aa0904..afdbaa1d9d 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -11,6 +11,7 @@ #include "alloc-util.h" #include "dbus-mount.h" +#include "dbus-unit.h" #include "device.h" #include "escape.h" #include "exit-status.h" @@ -640,6 +641,9 @@ static void mount_set_state(Mount *m, MountState state) { MountState old_state; assert(m); + if (m->state != state) + bus_unit_send_pending_change_signal(UNIT(m), false); + old_state = m->state; m->state = state; diff --git a/src/core/path.c b/src/core/path.c index 01019b0cf7..831e49df29 100644 --- a/src/core/path.c +++ b/src/core/path.c @@ -8,6 +8,7 @@ #include "bus-error.h" #include "bus-util.h" #include "dbus-path.h" +#include "dbus-unit.h" #include "fd-util.h" #include "fs-util.h" #include "glob-util.h" @@ -410,6 +411,9 @@ static void path_set_state(Path *p, PathState state) { PathState old_state; assert(p); + if (p->state != state) + bus_unit_send_pending_change_signal(UNIT(p), false); + old_state = p->state; p->state = state; diff --git a/src/core/scope.c b/src/core/scope.c index 151b8989a6..e478661f94 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -5,6 +5,7 @@ #include "alloc-util.h" #include "dbus-scope.h" +#include "dbus-unit.h" #include "load-dropin.h" #include "log.h" #include "scope.h" @@ -82,6 +83,9 @@ static void scope_set_state(Scope *s, ScopeState state) { ScopeState old_state; assert(s); + if (s->state != state) + bus_unit_send_pending_change_signal(UNIT(s), false); + old_state = s->state; s->state = state; diff --git a/src/core/service.c b/src/core/service.c index 964a7fd057..76f1e16069 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -12,6 +12,7 @@ #include "bus-kernel.h" #include "bus-util.h" #include "dbus-service.h" +#include "dbus-unit.h" #include "def.h" #include "env-util.h" #include "escape.h" @@ -1035,6 +1036,9 @@ static void service_set_state(Service *s, ServiceState state) { assert(s); + if (s->state != state) + bus_unit_send_pending_change_signal(UNIT(s), false); + table = s->type == SERVICE_IDLE ? state_translation_table_idle : state_translation_table; old_state = s->state; diff --git a/src/core/slice.c b/src/core/slice.c index dc087680e1..15b18bcad3 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -4,6 +4,7 @@ #include "alloc-util.h" #include "dbus-slice.h" +#include "dbus-unit.h" #include "log.h" #include "serialize.h" #include "slice.h" @@ -29,6 +30,9 @@ static void slice_set_state(Slice *t, SliceState state) { SliceState old_state; assert(t); + if (t->state != state) + bus_unit_send_pending_change_signal(UNIT(t), false); + old_state = t->state; t->state = state; diff --git a/src/core/socket.c b/src/core/socket.c index f725c9eb00..d3dc0a3216 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -17,6 +17,7 @@ #include "bus-util.h" #include "copy.h" #include "dbus-socket.h" +#include "dbus-unit.h" #include "def.h" #include "exit-status.h" #include "fd-util.h" @@ -1748,6 +1749,9 @@ static void socket_set_state(Socket *s, SocketState state) { SocketState old_state; assert(s); + if (s->state != state) + bus_unit_send_pending_change_signal(UNIT(s), false); + old_state = s->state; s->state = state; diff --git a/src/core/swap.c b/src/core/swap.c index db806fe0bb..90207a48fa 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -9,6 +9,7 @@ #include "alloc-util.h" #include "dbus-swap.h" +#include "dbus-unit.h" #include "device-private.h" #include "device-util.h" #include "device.h" @@ -480,6 +481,9 @@ static void swap_set_state(Swap *s, SwapState state) { assert(s); + if (s->state != state) + bus_unit_send_pending_change_signal(UNIT(s), false); + old_state = s->state; s->state = state; diff --git a/src/core/target.c b/src/core/target.c index b8b8e32805..421a304c73 100644 --- a/src/core/target.c +++ b/src/core/target.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include "dbus-target.h" +#include "dbus-unit.h" #include "log.h" #include "serialize.h" #include "special.h" @@ -18,6 +19,9 @@ static void target_set_state(Target *t, TargetState state) { TargetState old_state; assert(t); + if (t->state != state) + bus_unit_send_pending_change_signal(UNIT(t), false); + old_state = t->state; t->state = state; diff --git a/src/core/timer.c b/src/core/timer.c index 1527aab158..d9ba2f76b3 100644 --- a/src/core/timer.c +++ b/src/core/timer.c @@ -6,6 +6,7 @@ #include "bus-error.h" #include "bus-util.h" #include "dbus-timer.h" +#include "dbus-unit.h" #include "fs-util.h" #include "parse-util.h" #include "random-util.h" @@ -247,6 +248,9 @@ static void timer_set_state(Timer *t, TimerState state) { TimerState old_state; assert(t); + if (t->state != state) + bus_unit_send_pending_change_signal(UNIT(t), false); + old_state = t->state; t->state = state; From 17407bc28d731cd3efbfc1bf56cd963585e82fe9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 16:33:19 +0100 Subject: [PATCH 03/11] core: before sending out a job new/change/removal message, send out unit change message for job's unit We always want the state of the unit to be reflected first to the client before we claim the job has changed state, after all the job is the request to change unit state, and thus job changes are kinda the confirmation that the state changed as requested. --- src/core/dbus-job.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/core/dbus-job.c b/src/core/dbus-job.c index 20d890b36c..acb9f55a5e 100644 --- a/src/core/dbus-job.c +++ b/src/core/dbus-job.c @@ -4,6 +4,7 @@ #include "alloc-util.h" #include "dbus-job.h" +#include "dbus-unit.h" #include "dbus.h" #include "job.h" #include "log.h" @@ -173,6 +174,9 @@ void bus_job_send_change_signal(Job *j) { assert(j); + /* Make sure that any change signal on the unit is reflected before we send out the change signal on the job */ + bus_unit_send_pending_change_signal(j->unit, true); + if (j->in_dbus_queue) { LIST_REMOVE(dbus_queue, j->manager->dbus_job_queue, j); j->in_dbus_queue = false; @@ -222,6 +226,9 @@ void bus_job_send_removed_signal(Job *j) { if (!j->sent_dbus_new_signal) bus_job_send_change_signal(j); + /* Make sure that any change signal on the unit is reflected before we send out the change signal on the job */ + bus_unit_send_pending_change_signal(j->unit, true); + r = bus_foreach_bus(j->manager, j->bus_track, send_removed_signal, j); if (r < 0) log_debug_errno(r, "Failed to send job remove signal for %u: %m", j->id); From 13142276c1bbb6562aedeb5e828a00c9b6b45d96 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 18:48:20 +0100 Subject: [PATCH 04/11] core: before returning new job path to clients, force out JobNew signals When a client requests a new job, let's make sure we for out the JobNew signals for it, before we return successfully from the method call. After all we shouldn't return a path that is not announced yet, as announcement of jobs should be considered part of the job setup. --- src/core/dbus-job.c | 15 +++++++++++++++ src/core/dbus-job.h | 1 + src/core/dbus-unit.c | 3 +++ 3 files changed, 19 insertions(+) diff --git a/src/core/dbus-job.c b/src/core/dbus-job.c index acb9f55a5e..d11e58b51d 100644 --- a/src/core/dbus-job.c +++ b/src/core/dbus-job.c @@ -189,6 +189,21 @@ void bus_job_send_change_signal(Job *j) { j->sent_dbus_new_signal = true; } +void bus_job_send_pending_change_signal(Job *j, bool including_new) { + assert(j); + + if (!j->in_dbus_queue) + return; + + if (!j->sent_dbus_new_signal && !including_new) + return; + + if (MANAGER_IS_RELOADING(j->unit->manager)) + return; + + bus_job_send_change_signal(j); +} + static int send_removed_signal(sd_bus *bus, void *userdata) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; _cleanup_free_ char *p = NULL; diff --git a/src/core/dbus-job.h b/src/core/dbus-job.h index 3cc60f22ee..c9f6fc7187 100644 --- a/src/core/dbus-job.h +++ b/src/core/dbus-job.h @@ -12,6 +12,7 @@ int bus_job_method_cancel(sd_bus_message *message, void *job, sd_bus_error *erro int bus_job_method_get_waiting_jobs(sd_bus_message *message, void *userdata, sd_bus_error *error); void bus_job_send_change_signal(Job *j); +void bus_job_send_pending_change_signal(Job *j, bool including_new); void bus_job_send_removed_signal(Job *j); int bus_job_coldplug_bus_track(Job *j); diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 4cf7364c50..5665bf97a8 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -1321,6 +1321,9 @@ int bus_unit_queue_job( if (!path) return -ENOMEM; + /* Before we send the method reply, force out the announcement JobNew for this job */ + bus_job_send_pending_change_signal(j, true); + return sd_bus_reply_method_return(message, "o", path); } From e18f8852f37f1156c006e5907e099f89b4c67b29 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 16:34:59 +0100 Subject: [PATCH 05/11] core: invalidate invidual Assert/Condition properties when sending out change messages Let's inform the clients about assert/condition property changes as they happen, it's basically for free because assert/condition property changes generally coincide with other unit state changes (after all these checks are done on unit_start()) --- src/core/dbus-unit.c | 4 ++-- src/core/unit.c | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 5665bf97a8..f398dabc8d 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -662,8 +662,8 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("AssertResult", "b", bus_property_get_bool, offsetof(Unit, assert_result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), BUS_PROPERTY_DUAL_TIMESTAMP("ConditionTimestamp", offsetof(Unit, condition_timestamp), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), BUS_PROPERTY_DUAL_TIMESTAMP("AssertTimestamp", offsetof(Unit, assert_timestamp), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), - SD_BUS_PROPERTY("Conditions", "a(sbbsi)", property_get_conditions, offsetof(Unit, conditions), 0), - SD_BUS_PROPERTY("Asserts", "a(sbbsi)", property_get_conditions, offsetof(Unit, asserts), 0), + SD_BUS_PROPERTY("Conditions", "a(sbbsi)", property_get_conditions, offsetof(Unit, conditions), SD_BUS_VTABLE_PROPERTY_EMITS_INVALIDATION), + SD_BUS_PROPERTY("Asserts", "a(sbbsi)", property_get_conditions, offsetof(Unit, asserts), SD_BUS_VTABLE_PROPERTY_EMITS_INVALIDATION), SD_BUS_PROPERTY("LoadError", "(ss)", property_get_load_error, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Transient", "b", bus_property_get_bool, offsetof(Unit, transient), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Perpetual", "b", bus_property_get_bool, offsetof(Unit, perpetual), SD_BUS_VTABLE_PROPERTY_CONST), diff --git a/src/core/unit.c b/src/core/unit.c index cc4ae50516..175c56f48a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1637,6 +1637,8 @@ static bool unit_condition_test(Unit *u) { dual_timestamp_get(&u->condition_timestamp); u->condition_result = unit_condition_test_list(u, u->conditions, condition_type_to_string); + unit_add_to_dbus_queue(u); + return u->condition_result; } @@ -1646,6 +1648,8 @@ static bool unit_assert_test(Unit *u) { dual_timestamp_get(&u->assert_timestamp); u->assert_result = unit_condition_test_list(u, u->asserts, assert_type_to_string); + unit_add_to_dbus_queue(u); + return u->assert_result; } From af92c603bb820235b080b26c4fe452369d6be67c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 16:39:18 +0100 Subject: [PATCH 06/11] core: send out unit change events when a new invocation ID is acquired It's free, as this generally coincides with unit_start(), but let's make this clean and explicit. --- src/core/dbus-unit.c | 2 +- src/core/unit.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index f398dabc8d..ca81f6acc7 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -675,7 +675,7 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("SuccessAction", "s", property_get_emergency_action, offsetof(Unit, success_action), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("SuccessActionExitStatus", "i", bus_property_get_int, offsetof(Unit, success_action_exit_status), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("RebootArgument", "s", NULL, offsetof(Unit, reboot_arg), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("InvocationID", "ay", bus_property_get_id128, offsetof(Unit, invocation_id), 0), + SD_BUS_PROPERTY("InvocationID", "ay", bus_property_get_id128, offsetof(Unit, invocation_id), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), SD_BUS_PROPERTY("CollectMode", "s", property_get_collect_mode, offsetof(Unit, collect_mode), 0), SD_BUS_PROPERTY("Refs", "as", property_get_refs, 0, 0), diff --git a/src/core/unit.c b/src/core/unit.c index 175c56f48a..9f715b164c 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4986,6 +4986,7 @@ int unit_acquire_invocation_id(Unit *u) { if (r < 0) return log_unit_error_errno(u, r, "Failed to set invocation ID for unit: %m"); + unit_add_to_dbus_queue(u); return 0; } From 641e0d7a1bf4e24f5bf5f99a2d74a97635335c8e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 16:40:13 +0100 Subject: [PATCH 07/11] core: clarify that the CollectMode bus property is constant it's configured from unit files only, and hence is constant. --- src/core/dbus-unit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index ca81f6acc7..968166ee60 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -676,7 +676,7 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("SuccessActionExitStatus", "i", bus_property_get_int, offsetof(Unit, success_action_exit_status), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("RebootArgument", "s", NULL, offsetof(Unit, reboot_arg), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("InvocationID", "ay", bus_property_get_id128, offsetof(Unit, invocation_id), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), - SD_BUS_PROPERTY("CollectMode", "s", property_get_collect_mode, offsetof(Unit, collect_mode), 0), + SD_BUS_PROPERTY("CollectMode", "s", property_get_collect_mode, offsetof(Unit, collect_mode), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Refs", "as", property_get_refs, 0, 0), SD_BUS_METHOD("Start", "s", "o", method_start, SD_BUS_VTABLE_UNPRIVILEGED), From e6d05912cb1785d8c75eb40545beb8a7c6753cb9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 18:48:32 +0100 Subject: [PATCH 08/11] core: when we install a job, announce this via the bus Whenever we enqueue a job, we should announce this on the bus, hence add both the job and the unit to the dbus queues. (Why both? The former should be obvious, the latter because we send out Job properties). In most cases adding these to the queue is not necessary, as other properties tend to change at the same time and result in a change being sent out. However, let's clean this up and make it explicit. --- src/core/job.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/job.c b/src/core/job.c index 2a630356bf..61ca6eb433 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -236,6 +236,9 @@ Job* job_install(Job *j) { job_add_to_gc_queue(j); + job_add_to_dbus_queue(j); /* announce this job to clients */ + unit_add_to_dbus_queue(j->unit); /* The Job property of the unit has changed now */ + return j; } From 3c4832ada41466b87203ad145af1af2b0040dcf5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 18:48:52 +0100 Subject: [PATCH 09/11] core: enqueue unit earlier when state changes Previously, we'd enqueue a unit to the dbus queue whenever the state changed, after we processed the state change fully. This commit to the beginning of the state change. This has the benefit that when the state change causes a job to complete the unit is already in the dbus queue, and thus we get the guarantee that any unit change can be sent out to clients before the job change. --- src/core/unit.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/core/unit.c b/src/core/unit.c index 9f715b164c..7b8e0e37c7 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2341,6 +2341,10 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag m = u->manager; + /* Let's enqueue the change signal early. In case this unit has a job associated we want that this unit is in + * the bus queue, so that any job change signal queued will force out the unit change signal first. */ + unit_add_to_dbus_queue(u); + /* Update timestamps for state changes */ if (!MANAGER_IS_RELOADING(m)) { dual_timestamp_get(&u->state_change_timestamp); @@ -2499,7 +2503,6 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag } } - unit_add_to_dbus_queue(u); unit_add_to_gc_queue(u); } From 109b0cbf24443b54109a1b6dac010bdf3b96d93c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 18:39:11 +0100 Subject: [PATCH 10/11] systemctl: rework --wait logic Let's not honour PropertiesChanged signals unless the Jobs properties is empy. After all we shouldn't consider a service finished unless its state is inactive/failed *and* no job is queued for it anymore. --- src/systemctl/systemctl.c | 68 ++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 22 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 87ae4eb5d7..0013fd5bfa 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2802,63 +2802,87 @@ static void wait_context_free(WaitContext *c) { } static int on_properties_changed(sd_bus_message *m, void *userdata, sd_bus_error *error) { + const char *path, *interface, *active_state = NULL, *job_path = NULL; WaitContext *c = userdata; - const char *path; + bool is_failed; int r; + /* Called whenever we get a PropertiesChanged signal. Checks if ActiveState changed to inactive/failed. + * + * Signal parameters: (s interface, a{sv} changed_properties, as invalidated_properties) */ + path = sd_bus_message_get_path(m); if (!set_contains(c->unit_paths, path)) return 0; - /* Check if ActiveState changed to inactive/failed */ - /* (s interface, a{sv} changed_properties, as invalidated_properties) */ - r = sd_bus_message_skip(m, "s"); + r = sd_bus_message_read(m, "s", &interface); if (r < 0) return bus_log_parse_error(r); + if (!streq(interface, "org.freedesktop.systemd1.Unit")) /* ActiveState is on the Unit interface */ + return 0; + r = sd_bus_message_enter_container(m, SD_BUS_TYPE_ARRAY, "{sv}"); if (r < 0) return bus_log_parse_error(r); - while ((r = sd_bus_message_enter_container(m, SD_BUS_TYPE_DICT_ENTRY, "sv")) > 0) { + for (;;) { const char *s; - r = sd_bus_message_read(m, "s", &s); + r = sd_bus_message_enter_container(m, SD_BUS_TYPE_DICT_ENTRY, "sv"); + if (r < 0) + return bus_log_parse_error(r); + if (r == 0) /* end of array */ + break; + + r = sd_bus_message_read(m, "s", &s); /* Property name */ if (r < 0) return bus_log_parse_error(r); if (streq(s, "ActiveState")) { - bool is_failed; - - r = sd_bus_message_enter_container(m, SD_BUS_TYPE_VARIANT, "s"); + r = sd_bus_message_read(m, "v", "s", &active_state); if (r < 0) return bus_log_parse_error(r); - r = sd_bus_message_read(m, "s", &s); + if (job_path) /* Found everything we need */ + break; + + } else if (streq(s, "Job")) { + uint32_t job_id; + + r = sd_bus_message_read(m, "v", "(uo)", &job_id, &job_path); if (r < 0) return bus_log_parse_error(r); - is_failed = streq(s, "failed"); - if (streq(s, "inactive") || is_failed) { - log_debug("%s became %s, dropping from --wait tracking", path, s); - free(set_remove(c->unit_paths, path)); - c->any_failed = c->any_failed || is_failed; - } else - log_debug("ActiveState on %s changed to %s", path, s); + /* There's still a job pending for this unit, let's ignore this for now, and return right-away. */ + if (job_id != 0) + return 0; + + if (active_state) /* Found everything we need */ + break; - break; /* no need to dissect the rest of the message */ } else { - /* other property */ - r = sd_bus_message_skip(m, "v"); + r = sd_bus_message_skip(m, "v"); /* Other property */ if (r < 0) return bus_log_parse_error(r); } + r = sd_bus_message_exit_container(m); if (r < 0) return bus_log_parse_error(r); } - if (r < 0) - return bus_log_parse_error(r); + + /* If this didn't contain the ActiveState property we can't do anything */ + if (!active_state) + return 0; + + is_failed = streq(active_state, "failed"); + if (streq(active_state, "inactive") || is_failed) { + log_debug("%s became %s, dropping from --wait tracking", path, active_state); + free(set_remove(c->unit_paths, path)); + c->any_failed = c->any_failed || is_failed; + } else + log_debug("ActiveState on %s changed to %s", path, active_state); if (set_isempty(c->unit_paths)) sd_event_exit(c->event, EXIT_SUCCESS); From db3cea2219aef0187cb4fc511cff43c0500358b8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 17:19:41 +0100 Subject: [PATCH 11/11] update TODO --- TODO | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/TODO b/TODO index cafd75a01d..41ad24d3bb 100644 --- a/TODO +++ b/TODO @@ -23,6 +23,10 @@ Janitorial Clean-ups: Features: +* when importing an fs tree with machined, optionally apply userns-rec-chown + +* when importing an fs tree with machined, complain if image is not an OS + * when we fork off generators and such, lower LIMIT_NOFILE soft limit to 1K * rework seccomp/nnp logic that that even if User= is used in combination with