From 33fe0afe9af56af67ab72cf407cb41a8628e0e06 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2017 12:51:54 +0100 Subject: [PATCH 1/4] core: serialize the "controller" field in scope units We forgot to serialize it previously, hence daemon reload flushed it out, since we also didn't write it to any unit file... --- src/core/scope.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/core/scope.c b/src/core/scope.c index 444c00be92..05b2ec31d1 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -411,11 +411,16 @@ static int scope_serialize(Unit *u, FILE *f, FDSet *fds) { unit_serialize_item(u, f, "state", scope_state_to_string(s->state)); unit_serialize_item(u, f, "was-abandoned", yes_no(s->was_abandoned)); + + if (s->controller) + unit_serialize_item(u, f, "controller", s->controller); + return 0; } static int scope_deserialize_item(Unit *u, const char *key, const char *value, FDSet *fds) { Scope *s = SCOPE(u); + int r; assert(u); assert(key); @@ -439,6 +444,12 @@ static int scope_deserialize_item(Unit *u, const char *key, const char *value, F log_unit_debug(u, "Failed to parse boolean value: %s", value); else s->was_abandoned = k; + } else if (streq(key, "controller")) { + + r = free_and_strdup(&s->controller, value); + if (r < 0) + log_oom(); + } else log_unit_debug(u, "Unknown serialization key: %s", key); From abdb9b08f6b0da45603a5928eb884a8d012127a7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2017 19:27:47 +0100 Subject: [PATCH 2/4] nspawn: make use of the RequestStop logic of scope units Since time began, scope units had a concept of "Controllers", a bus peer that would be notified when somebody requested a unit to stop. None of our code used that facility so far, let's change that. This way, nspawn can print a nice message when somebody invokes "systemctl stop" on the container's scope unit, and then react with the right action to shut it down. --- src/nspawn/nspawn-register.c | 45 +++++++++++++++++------- src/nspawn/nspawn-register.h | 6 ++-- src/nspawn/nspawn.c | 66 ++++++++++++++++++++++++++++++++++-- 3 files changed, 98 insertions(+), 19 deletions(-) diff --git a/src/nspawn/nspawn-register.c b/src/nspawn/nspawn-register.c index 9d58953515..ef9db31df7 100644 --- a/src/nspawn/nspawn-register.c +++ b/src/nspawn/nspawn-register.c @@ -99,7 +99,26 @@ static int append_machine_properties( return 0; } +static int append_controller_property(sd_bus *bus, sd_bus_message *m) { + const char *unique; + int r; + + assert(bus); + assert(m); + + r = sd_bus_get_unique_name(bus, &unique); + if (r < 0) + return log_error_errno(r, "Failed to get unique name: %m"); + + r = sd_bus_message_append(m, "(sv)", "Controller", "s", unique); + if (r < 0) + return bus_log_create_error(r); + + return 0; +} + int register_machine( + sd_bus *bus, const char *machine_name, pid_t pid, const char *directory, @@ -114,12 +133,9 @@ int register_machine( const char *service) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; int r; - r = sd_bus_default_system(&bus); - if (r < 0) - return log_error_errno(r, "Failed to open system bus: %m"); + assert(bus); if (keep_unit) { r = sd_bus_call_method( @@ -174,6 +190,10 @@ int register_machine( return bus_log_create_error(r); } + r = append_controller_property(bus, m); + if (r < 0) + return r; + r = append_machine_properties( m, mounts, @@ -202,16 +222,13 @@ int register_machine( return 0; } -int terminate_machine(pid_t pid) { +int terminate_machine(sd_bus *bus, pid_t pid) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; - _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; const char *path; int r; - r = sd_bus_default_system(&bus); - if (r < 0) - return log_error_errno(r, "Failed to open system bus: %m"); + assert(bus); r = sd_bus_call_method( bus, @@ -253,6 +270,7 @@ int terminate_machine(pid_t pid) { } int allocate_scope( + sd_bus *bus, const char *machine_name, pid_t pid, const char *slice, @@ -263,16 +281,13 @@ int allocate_scope( _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; - _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *w = NULL; _cleanup_free_ char *scope = NULL; const char *description, *object; int r; - r = sd_bus_default_system(&bus); - if (r < 0) - return log_error_errno(r, "Failed to open system bus: %m"); + assert(bus); r = bus_wait_for_jobs_new(bus, &w); if (r < 0) @@ -311,6 +326,10 @@ int allocate_scope( if (r < 0) return bus_log_create_error(r); + r = append_controller_property(bus, m); + if (r < 0) + return r; + r = append_machine_properties( m, mounts, diff --git a/src/nspawn/nspawn-register.h b/src/nspawn/nspawn-register.h index 76c13bb356..fa3644c443 100644 --- a/src/nspawn/nspawn-register.h +++ b/src/nspawn/nspawn-register.h @@ -26,7 +26,7 @@ #include "nspawn-mount.h" -int register_machine(const char *machine_name, pid_t pid, const char *directory, sd_id128_t uuid, int local_ifindex, const char *slice, CustomMount *mounts, unsigned n_mounts, int kill_signal, char **properties, bool keep_unit, const char *service); -int terminate_machine(pid_t pid); +int register_machine(sd_bus *bus, const char *machine_name, pid_t pid, const char *directory, sd_id128_t uuid, int local_ifindex, const char *slice, CustomMount *mounts, unsigned n_mounts, int kill_signal, char **properties, bool keep_unit, const char *service); +int terminate_machine(sd_bus *bus, pid_t pid); -int allocate_scope(const char *machine_name, pid_t pid, const char *slice, CustomMount *mounts, unsigned n_mounts, int kill_signal, char **properties); +int allocate_scope(sd_bus *bus, const char *machine_name, pid_t pid, const char *slice, CustomMount *mounts, unsigned n_mounts, int kill_signal, char **properties); diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 3bf4d6ed1b..8fc39dd2ba 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -2039,18 +2039,27 @@ static int on_orderly_shutdown(sd_event_source *s, const struct signalfd_siginfo } static int on_sigchld(sd_event_source *s, const struct signalfd_siginfo *ssi, void *userdata) { + pid_t pid; + + assert(s); + assert(ssi); + + pid = PTR_TO_PID(userdata); + for (;;) { siginfo_t si = {}; + if (waitid(P_ALL, 0, &si, WNOHANG|WNOWAIT|WEXITED) < 0) return log_error_errno(errno, "Failed to waitid(): %m"); if (si.si_pid == 0) /* No pending children. */ break; - if (si.si_pid == PTR_TO_PID(userdata)) { + if (si.si_pid == pid) { /* The main process we care for has exited. Return from * signal handler but leave the zombie. */ sd_event_exit(sd_event_source_get_event(s), 0); break; } + /* Reap all other children. */ (void) waitid(P_PID, si.si_pid, &si, WNOHANG|WEXITED); } @@ -2058,6 +2067,24 @@ static int on_sigchld(sd_event_source *s, const struct signalfd_siginfo *ssi, vo return 0; } +static int on_request_stop(sd_bus_message *m, void *userdata, sd_bus_error *error) { + pid_t pid; + + assert(m); + + pid = PTR_TO_PID(userdata); + + if (arg_kill_signal > 0) { + log_info("Container termination requested. Attempting to halt container."); + (void) kill(pid, arg_kill_signal); + } else { + log_info("Container termination requested. Exiting."); + sd_event_exit(sd_bus_get_event(sd_bus_message_get_bus(m)), 0); + } + + return 0; +} + static int determine_names(void) { int r; @@ -3214,6 +3241,7 @@ static int run(int master, _cleanup_(sd_event_unrefp) sd_event *event = NULL; _cleanup_(pty_forward_freep) PTYForward *forward = NULL; _cleanup_(sd_netlink_unrefp) sd_netlink *rtnl = NULL; + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; ContainerStatus container_status = 0; char last_char = 0; int ifi = 0, r; @@ -3444,8 +3472,31 @@ static int run(int master, return r; } + if (arg_register || !arg_keep_unit) { + r = sd_bus_default_system(&bus); + if (r < 0) + return log_error_errno(r, "Failed to open system bus: %m"); + } + + if (!arg_keep_unit) { + /* When a new scope is created for this container, then we'll be registered as its controller, in which + * case PID 1 will send us a friendly RequestStop signal, when it is asked to terminate the + * scope. Let's hook into that, and cleanly shut down the container, and print a friendly message. */ + + r = sd_bus_add_match(bus, NULL, + "type='signal'," + "sender='org.freedesktop.systemd1'," + "interface='org.freedesktop.systemd1.Scope'," + "member='RequestStop'", + on_request_stop, PID_TO_PTR(*pid)); + if (r < 0) + return log_error_errno(r, "Failed to install request stop match: %m"); + } + if (arg_register) { + r = register_machine( + bus, arg_machine, *pid, arg_directory, @@ -3459,8 +3510,11 @@ static int run(int master, arg_container_service_name); if (r < 0) return r; + } else if (!arg_keep_unit) { + r = allocate_scope( + bus, arg_machine, *pid, arg_slice, @@ -3506,6 +3560,12 @@ static int run(int master, if (r < 0) return log_error_errno(r, "Failed to get default event source: %m"); + if (bus) { + r = sd_bus_attach_event(bus, event, 0); + if (r < 0) + return log_error_errno(r, "Failed to attach bus to event loop: %m"); + } + r = setup_sd_notify_parent(event, notify_socket, PID_TO_PTR(*pid), ¬ify_event_source); if (r < 0) return r; @@ -3567,8 +3627,8 @@ static int run(int master, putc('\n', stdout); /* Kill if it is not dead yet anyway */ - if (arg_register && !arg_keep_unit) - terminate_machine(*pid); + if (arg_register && !arg_keep_unit && bus) + terminate_machine(bus, *pid); /* Normally redundant, but better safe than sorry */ (void) kill(*pid, SIGKILL); From f2c49c86582300e5fadc0297b29230d4caa1d8fa Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2017 20:02:00 +0100 Subject: [PATCH 3/4] core: refuse accepting a scope controller unless we are called on the API bus Let's make sure clients get early errors if they try something weird. --- src/core/dbus-scope.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/core/dbus-scope.c b/src/core/dbus-scope.c index 2d5baab4ce..46222efcb7 100644 --- a/src/core/dbus-scope.c +++ b/src/core/dbus-scope.c @@ -119,6 +119,11 @@ static int bus_scope_set_transient_property( const char *controller; char *c; + /* We can't support direct connections with this, as direct connections know no service or unique name + * concept, but the Controller field stores exactly that. */ + if (sd_bus_message_get_bus(message) != UNIT(s)->manager->api_bus) + return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, "Sorry, Controller= logic only supported via the bus."); + r = sd_bus_message_read(message, "s", &controller); if (r < 0) return r; From 371c0b794e8e677bf792db392991ade30c48245c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2017 20:15:48 +0100 Subject: [PATCH 4/4] core: track scope controllers on the bus This watches controllers on the bus, and unsets them automatically when they disappear. Note that this is primarily a cosmetical fix. Since unique bus names are not recycled, there's strictly no need to forget about them, but it's a lot nicer to do so. --- src/core/dbus-scope.c | 39 ++++++++++++++++++++++++++++++++++++++- src/core/dbus-scope.h | 2 ++ src/core/scope.c | 15 +++++++++++---- src/core/scope.h | 2 ++ 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/src/core/dbus-scope.c b/src/core/dbus-scope.c index 46222efcb7..4737c6d406 100644 --- a/src/core/dbus-scope.c +++ b/src/core/dbus-scope.c @@ -61,7 +61,7 @@ static BUS_DEFINE_PROPERTY_GET_ENUM(property_get_result, scope_result, ScopeResu const sd_bus_vtable bus_scope_vtable[] = { SD_BUS_VTABLE_START(0), - SD_BUS_PROPERTY("Controller", "s", NULL, offsetof(Scope, controller), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("Controller", "s", NULL, offsetof(Scope, controller), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), SD_BUS_PROPERTY("TimeoutStopUSec", "t", bus_property_get_usec, offsetof(Scope, timeout_stop_usec), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Result", "s", property_get_result, offsetof(Scope, result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), SD_BUS_SIGNAL("RequestStop", NULL, 0), @@ -233,3 +233,40 @@ int bus_scope_send_request_stop(Scope *s) { return sd_bus_send_to(UNIT(s)->manager->api_bus, m, s->controller, NULL); } + +static int on_controller_gone(sd_bus_track *track, void *userdata) { + Scope *s = userdata; + + assert(track); + + if (s->controller) { + log_unit_debug(UNIT(s), "Controller %s disappeared from bus.", s->controller); + unit_add_to_dbus_queue(UNIT(s)); + s->controller = mfree(s->controller); + } + + s->controller_track = sd_bus_track_unref(s->controller_track); + + return 0; +} + +int bus_scope_track_controller(Scope *s) { + int r; + + assert(s); + + if (!s->controller || s->controller_track) + return 0; + + r = sd_bus_track_new(UNIT(s)->manager->api_bus, &s->controller_track, on_controller_gone, s); + if (r < 0) + return r; + + r = sd_bus_track_add_name(s->controller_track, s->controller); + if (r < 0) { + s->controller_track = sd_bus_track_unref(s->controller_track); + return r; + } + + return 0; +} diff --git a/src/core/dbus-scope.h b/src/core/dbus-scope.h index 8e4358204d..c80317c4b2 100644 --- a/src/core/dbus-scope.h +++ b/src/core/dbus-scope.h @@ -30,3 +30,5 @@ int bus_scope_set_property(Unit *u, const char *name, sd_bus_message *i, UnitSet int bus_scope_commit_properties(Unit *u); int bus_scope_send_request_stop(Scope *s); + +int bus_scope_track_controller(Scope *s); diff --git a/src/core/scope.c b/src/core/scope.c index 05b2ec31d1..e0e25673cb 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -59,7 +59,8 @@ static void scope_done(Unit *u) { assert(u); - free(s->controller); + s->controller = mfree(s->controller); + s->controller_track = sd_bus_track_unref(s->controller_track); s->timer_event_source = sd_event_source_unref(s->timer_event_source); } @@ -229,6 +230,8 @@ static int scope_coldplug(Unit *u) { if (!IN_SET(s->deserialized_state, SCOPE_DEAD, SCOPE_FAILED)) unit_watch_all_pids(UNIT(s)); + bus_scope_track_controller(s); + scope_set_state(s, s->deserialized_state); return 0; } @@ -272,9 +275,8 @@ static void scope_enter_signal(Scope *s, ScopeState state, ScopeResult f) { unit_watch_all_pids(UNIT(s)); - /* If we have a controller set let's ask the controller nicely - * to terminate the scope, instead of us going directly into - * SIGTERM berserk mode */ + /* If we have a controller set let's ask the controller nicely to terminate the scope, instead of us going + * directly into SIGTERM berserk mode */ if (state == SCOPE_STOP_SIGTERM) skip_signal = bus_scope_send_request_stop(s) > 0; @@ -332,6 +334,8 @@ static int scope_start(Unit *u) { if (!u->transient && !MANAGER_IS_RELOADING(u->manager)) return -ENOENT; + (void) bus_scope_track_controller(s); + r = unit_acquire_invocation_id(u); if (r < 0) return r; @@ -536,7 +540,10 @@ int scope_abandon(Scope *s) { return -ESTALE; s->was_abandoned = true; + s->controller = mfree(s->controller); + s->controller_track = sd_bus_track_unref(s->controller_track); + scope_set_state(s, SCOPE_ABANDONED); /* The client is no longer watching the remaining processes, diff --git a/src/core/scope.h b/src/core/scope.h index 7e8c47dd42..ca7c6c868f 100644 --- a/src/core/scope.h +++ b/src/core/scope.h @@ -46,6 +46,8 @@ struct Scope { usec_t timeout_stop_usec; char *controller; + sd_bus_track *controller_track; + bool was_abandoned; sd_event_source *timer_event_source;