From ebc57b89c65dbbfc68b8c395f3c86c1e4a3129d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Oct 2019 09:39:25 +0100 Subject: [PATCH 1/5] pid1: log the reason why restart will or will not happen I was trying to figure out why the restart was not happening, and it wasn't at all obvious. Let's add a nice debug message. --- src/core/service.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index bfbfa4be655..b024c94e89d 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1669,21 +1669,28 @@ static int cgroup_good(Service *s) { return r == 0; } -static bool service_shall_restart(Service *s) { +static bool service_shall_restart(Service *s, const char **reason) { assert(s); /* Don't restart after manual stops */ - if (s->forbid_restart) + if (s->forbid_restart) { + *reason = "manual stop"; return false; + } /* Never restart if this is configured as special exception */ - if (exit_status_set_test(&s->restart_prevent_status, s->main_exec_status.code, s->main_exec_status.status)) + if (exit_status_set_test(&s->restart_prevent_status, s->main_exec_status.code, s->main_exec_status.status)) { + *reason = "prevented by exit status"; return false; + } /* Restart if the exit code/status are configured as restart triggers */ - if (exit_status_set_test(&s->restart_force_status, s->main_exec_status.code, s->main_exec_status.status)) + if (exit_status_set_test(&s->restart_force_status, s->main_exec_status.code, s->main_exec_status.status)) { + *reason = "forced by exit status"; return true; + } + *reason = "restart setting"; switch (s->restart) { case SERVICE_RESTART_NO: @@ -1754,8 +1761,19 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart) end_state = SERVICE_FAILED; } - if (allow_restart && service_shall_restart(s)) - s->will_auto_restart = true; + if (!allow_restart) + log_unit_debug(UNIT(s), "Service restart not allowed."); + else { + const char *reason; + bool shall_restart; + + shall_restart = service_shall_restart(s, &reason); + log_unit_debug(UNIT(s), "Service will %srestart (%s)", + shall_restart ? "" : "not ", + reason); + if (shall_restart) + s->will_auto_restart = true; + } /* Make sure service_release_resources() doesn't destroy our FD store, while we are changing through * SERVICE_FAILED/SERVICE_DEAD before entering into SERVICE_AUTO_RESTART. */ From ef8ff92e8ae014d352de381ff0a83e25e6f00efb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Oct 2019 09:43:07 +0100 Subject: [PATCH 2/5] machined: add UnregisterMachine method This is the opposite of RegisterMachine: machined knows that the machine is "gone", but doesn't do anything on its own. We already had TerminateMachine, but that would stop the unit, which isn't always wanted. --- src/machine/machine-dbus.c | 28 +++++++++++++++++++++++ src/machine/machine-dbus.h | 1 + src/machine/machine.c | 5 +++- src/machine/machined-dbus.c | 5 ++++ src/machine/org.freedesktop.machine1.conf | 4 ++++ 5 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/machine/machine-dbus.c b/src/machine/machine-dbus.c index 0d58b5eb8b6..31b54680ec4 100644 --- a/src/machine/machine-dbus.c +++ b/src/machine/machine-dbus.c @@ -62,6 +62,34 @@ static int property_get_netif( return sd_bus_message_append_array(reply, 'i', m->netif, m->n_netif * sizeof(int)); } +int bus_machine_method_unregister(sd_bus_message *message, void *userdata, sd_bus_error *error) { + Machine *m = userdata; + int r; + + assert(message); + assert(m); + + r = bus_verify_polkit_async( + message, + CAP_KILL, + "org.freedesktop.machine1.manage-machines", + NULL, + false, + UID_INVALID, + &m->manager->polkit_registry, + error); + if (r < 0) + return r; + if (r == 0) + return 1; /* Will call us back */ + + r = machine_finalize(m); + if (r < 0) + return r; + + return sd_bus_reply_method_return(message, NULL); +} + int bus_machine_method_terminate(sd_bus_message *message, void *userdata, sd_bus_error *error) { Machine *m = userdata; int r; diff --git a/src/machine/machine-dbus.h b/src/machine/machine-dbus.h index f880803dc21..d9f3c59cead 100644 --- a/src/machine/machine-dbus.h +++ b/src/machine/machine-dbus.h @@ -11,6 +11,7 @@ char *machine_bus_path(Machine *s); int machine_object_find(sd_bus *bus, const char *path, const char *interface, void *userdata, void **found, sd_bus_error *error); int machine_node_enumerator(sd_bus *bus, const char *path, void *userdata, char ***nodes, sd_bus_error *error); +int bus_machine_method_unregister(sd_bus_message *message, void *userdata, sd_bus_error *error); int bus_machine_method_terminate(sd_bus_message *message, void *userdata, sd_bus_error *error); int bus_machine_method_kill(sd_bus_message *message, void *userdata, sd_bus_error *error); int bus_machine_method_get_addresses(sd_bus_message *message, void *userdata, sd_bus_error *error); diff --git a/src/machine/machine.c b/src/machine/machine.c index 4d07a7893a9..b5d017a6717 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -449,13 +449,16 @@ int machine_stop(Machine *m) { int machine_finalize(Machine *m) { assert(m); - if (m->started) + if (m->started) { log_struct(LOG_INFO, "MESSAGE_ID=" SD_MESSAGE_MACHINE_STOP_STR, "NAME=%s", m->name, "LEADER="PID_FMT, m->leader, LOG_MESSAGE("Machine %s terminated.", m->name)); + m->stopping = true; /* The machine is supposed to be going away. Don't try to kill it. */ + } + machine_unlink(m); machine_add_to_gc_queue(m); diff --git a/src/machine/machined-dbus.c b/src/machine/machined-dbus.c index 8031dafe158..5276398ab54 100644 --- a/src/machine/machined-dbus.c +++ b/src/machine/machined-dbus.c @@ -448,6 +448,10 @@ static int redirect_method_to_machine(sd_bus_message *message, Manager *m, sd_bu return method(message, machine, error); } +static int method_unregister_machine(sd_bus_message *message, void *userdata, sd_bus_error *error) { + return redirect_method_to_machine(message, userdata, error, bus_machine_method_unregister); +} + static int method_terminate_machine(sd_bus_message *message, void *userdata, sd_bus_error *error) { return redirect_method_to_machine(message, userdata, error, bus_machine_method_terminate); } @@ -1137,6 +1141,7 @@ const sd_bus_vtable manager_vtable[] = { SD_BUS_METHOD("CreateMachineWithNetwork", "sayssusaia(sv)", "o", method_create_machine_with_network, 0), SD_BUS_METHOD("RegisterMachine", "sayssus", "o", method_register_machine, 0), SD_BUS_METHOD("RegisterMachineWithNetwork", "sayssusai", "o", method_register_machine_with_network, 0), + SD_BUS_METHOD("UnregisterMachine", "s", NULL, method_unregister_machine, SD_BUS_VTABLE_UNPRIVILEGED), SD_BUS_METHOD("TerminateMachine", "s", NULL, method_terminate_machine, SD_BUS_VTABLE_UNPRIVILEGED), SD_BUS_METHOD("KillMachine", "ssi", NULL, method_kill_machine, SD_BUS_VTABLE_UNPRIVILEGED), SD_BUS_METHOD("GetMachineAddresses", "s", "a(iay)", method_get_machine_addresses, SD_BUS_VTABLE_UNPRIVILEGED), diff --git a/src/machine/org.freedesktop.machine1.conf b/src/machine/org.freedesktop.machine1.conf index 73b7e071031..eafbf6bdc05 100644 --- a/src/machine/org.freedesktop.machine1.conf +++ b/src/machine/org.freedesktop.machine1.conf @@ -76,6 +76,10 @@ send_interface="org.freedesktop.machine1.Manager" send_member="OpenMachineShell"/> + + From 0bb0a9faa7a9be917255648dcbc4b13132d90161 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Oct 2019 09:47:57 +0100 Subject: [PATCH 3/5] nspawn: when stopping the machine, just deregister the machine We already shut the machine down ourselves (and pid1 will also do cleanup for us after we exit if anything was left behind). No need for systemd-machined to try to stop the unit too. (This calls the new machined method. If we are running against an older machined, we will not deregister the machine. If we are simply exiting, machined should notice that the unit is gone on its own. If we are restarting, we will fail to register the machine after restart and fail. But this case was already broken, because machined would create a stop job, breaking the restart. So not doing anything with old machined should not make anything more broken than it already is.) Fixes #13766. --- src/nspawn/nspawn-register.c | 6 +++--- src/nspawn/nspawn-register.h | 2 +- src/nspawn/nspawn.c | 12 ++++++------ 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/nspawn/nspawn-register.c b/src/nspawn/nspawn-register.c index 8e2c3296653..7541c56d8fe 100644 --- a/src/nspawn/nspawn-register.c +++ b/src/nspawn/nspawn-register.c @@ -209,7 +209,7 @@ int register_machine( return 0; } -int terminate_machine( +int unregister_machine( sd_bus *bus, const char *machine_name) { @@ -223,13 +223,13 @@ int terminate_machine( "org.freedesktop.machine1", "/org/freedesktop/machine1", "org.freedesktop.machine1.Manager", - "TerminateMachine", + "UnregisterMachine", &error, NULL, "s", machine_name); if (r < 0) - log_debug("Failed to terminate machine: %s", bus_error_message(&error, r)); + log_debug("Failed to unregister machine: %s", bus_error_message(&error, r)); return 0; } diff --git a/src/nspawn/nspawn-register.h b/src/nspawn/nspawn-register.h index 65a3ae85a72..07cca7fadce 100644 --- a/src/nspawn/nspawn-register.h +++ b/src/nspawn/nspawn-register.h @@ -8,7 +8,7 @@ #include "nspawn-mount.h" 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, sd_bus_message *properties_message, bool keep_unit, const char *service); -int terminate_machine(sd_bus *bus, const char *machine_name); +int unregister_machine(sd_bus *bus, const char *machine_name); 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, sd_bus_message *properties_message); int terminate_scope(sd_bus *bus, const char *machine_name); diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index fed5c651ced..48244f3ae85 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -4551,12 +4551,8 @@ static int run_container( } /* Kill if it is not dead yet anyway */ - if (bus) { - if (arg_register) - terminate_machine(bus, arg_machine); - else if (!arg_keep_unit) - terminate_scope(bus, arg_machine); - } + if (!arg_register && !arg_keep_unit && bus) + terminate_scope(bus, arg_machine); /* Normally redundant, but better safe than sorry */ (void) kill(*pid, SIGKILL); @@ -4564,6 +4560,10 @@ static int run_container( r = wait_for_container(*pid, &container_status); *pid = 0; + /* Tell machined that we are gone. */ + if (bus) + (void) unregister_machine(bus, arg_machine); + if (r < 0) /* We failed to wait for the container, or the container exited abnormally. */ return r; From 62a3fc6d27d6c78ad2157cd1bf7884a922ea02d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Oct 2019 10:31:19 +0100 Subject: [PATCH 4/5] shared/logs-show: strip trailing carriage returns at EOL/EOF When showing logs from a container, we would fail to show various lines: Oct 29 09:50:51 krowka systemd-nspawn[61376]: Detected architecture x86-64. Oct 29 09:50:51 krowka systemd-nspawn[61376]: [1B blob data] Oct 29 09:50:51 krowka systemd-nspawn[61376]: Welcome to Fedora 32 (Rawhide)! Oct 29 09:50:51 krowka systemd-nspawn[61376]: [1B blob data] Those are only harmless \r characters that trail the line. We already replace tabs and strip various ansi characters that we deem inconsequential, so let's also strip trailing carriage returns. Non-trailing ones are different, because they change what would be displayed. --- src/basic/string-util.c | 25 ++++++++++++++++++++----- src/test/test-strip-tab-ansi.c | 15 +++++++++++++++ src/test/test-utf8.c | 3 +++ 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/basic/string-util.c b/src/basic/string-util.c index 9586b3940eb..c1828687557 100644 --- a/src/basic/string-util.c +++ b/src/basic/string-util.c @@ -743,7 +743,7 @@ static void advance_offsets( } char *strip_tab_ansi(char **ibuf, size_t *_isz, size_t highlight[2]) { - const char *i, *begin = NULL; + const char *begin = NULL; enum { STATE_OTHER, STATE_ESCAPE, @@ -751,7 +751,7 @@ char *strip_tab_ansi(char **ibuf, size_t *_isz, size_t highlight[2]) { STATE_CSO, } state = STATE_OTHER; char *obuf = NULL; - size_t osz = 0, isz, shift[2] = {}; + size_t osz = 0, isz, shift[2] = {}, n_carriage_returns = 0; FILE *f; assert(ibuf); @@ -762,6 +762,8 @@ char *strip_tab_ansi(char **ibuf, size_t *_isz, size_t highlight[2]) { * 1. Replaces TABs by 8 spaces * 2. Strips ANSI color sequences (a subset of CSI), i.e. ESC '[' … 'm' sequences * 3. Strips ANSI operating system sequences (CSO), i.e. ESC ']' … BEL sequences + * 4. Strip trailing \r characters (since they would "move the cursor", but have no + * other effect). * * Everything else will be left as it is. In particular other ANSI sequences are left as they are, as * are any other special characters. Truncated ANSI sequences are left-as is too. This call is @@ -777,14 +779,24 @@ char *strip_tab_ansi(char **ibuf, size_t *_isz, size_t highlight[2]) { if (!f) return NULL; - for (i = *ibuf; i < *ibuf + isz + 1; i++) { + for (const char *i = *ibuf; i < *ibuf + isz + 1; i++) { switch (state) { case STATE_OTHER: if (i >= *ibuf + isz) /* EOT */ break; - else if (*i == '\x1B') + + if (*i == '\r') { + n_carriage_returns++; + break; + } else if (*i == '\n') + /* Ignore carriage returns before new line */ + n_carriage_returns = 0; + for (; n_carriage_returns > 0; n_carriage_returns--) + fputc('\r', f); + + if (*i == '\x1B') state = STATE_ESCAPE; else if (*i == '\t') { fputs(" ", f); @@ -795,6 +807,8 @@ char *strip_tab_ansi(char **ibuf, size_t *_isz, size_t highlight[2]) { break; case STATE_ESCAPE: + assert(n_carriage_returns == 0); + if (i >= *ibuf + isz) { /* EOT */ fputc('\x1B', f); advance_offsets(i - *ibuf, highlight, shift, 1); @@ -815,6 +829,7 @@ char *strip_tab_ansi(char **ibuf, size_t *_isz, size_t highlight[2]) { break; case STATE_CSI: + assert(n_carriage_returns == 0); if (i >= *ibuf + isz || /* EOT … */ !strchr("01234567890;m", *i)) { /* … or invalid chars in sequence */ @@ -829,6 +844,7 @@ char *strip_tab_ansi(char **ibuf, size_t *_isz, size_t highlight[2]) { break; case STATE_CSO: + assert(n_carriage_returns == 0); if (i >= *ibuf + isz || /* EOT … */ (*i != '\a' && (uint8_t) *i < 32U) || (uint8_t) *i > 126U) { /* … or invalid chars in sequence */ @@ -848,7 +864,6 @@ char *strip_tab_ansi(char **ibuf, size_t *_isz, size_t highlight[2]) { fclose(f); return mfree(obuf); } - fclose(f); free_and_replace(*ibuf, obuf); diff --git a/src/test/test-strip-tab-ansi.c b/src/test/test-strip-tab-ansi.c index fae384ef9b6..8e305f37912 100644 --- a/src/test/test-strip-tab-ansi.c +++ b/src/test/test-strip-tab-ansi.c @@ -34,6 +34,21 @@ int main(int argc, char *argv[]) { assert_se(streq(p, "\x1B[waldo")); free(p); + assert_se(p = strdup("\r\rwaldo")); + assert_se(strip_tab_ansi(&p, NULL, NULL)); + assert_se(streq(p, "\r\rwaldo")); + free(p); + + assert_se(p = strdup("waldo\r\r")); + assert_se(strip_tab_ansi(&p, NULL, NULL)); + assert_se(streq(p, "waldo")); + free(p); + + assert_se(p = strdup("waldo\r\r\n\r\n")); + assert_se(strip_tab_ansi(&p, NULL, NULL)); + assert_se(streq(p, "waldo\n\n")); + free(p); + assert_se(terminal_urlify_path("/etc/fstab", "i am a fabulous link", &urlified) >= 0); assert_se(p = strjoin("something ", urlified, " something-else")); assert_se(q = strdup(p)); diff --git a/src/test/test-utf8.c b/src/test/test-utf8.c index b5c4e3dc343..8937f56237f 100644 --- a/src/test/test-utf8.c +++ b/src/test/test-utf8.c @@ -13,6 +13,9 @@ static void test_utf8_is_printable(void) { assert_se(utf8_is_printable("\342\204\242", 3)); assert_se(!utf8_is_printable("\341\204", 2)); assert_se(utf8_is_printable("ąę", 4)); + assert_se(!utf8_is_printable("\r", 1)); + assert_se(utf8_is_printable("\n", 1)); + assert_se(utf8_is_printable("\t", 1)); } static void test_utf8_is_valid(void) { From 2798430e00f7e9186cd3d0f97c1e43897181006f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Oct 2019 10:46:21 +0100 Subject: [PATCH 5/5] machined: only Unref units that we AddRef'd b92d0b4c5adef37e9de8f6cc22a0e27b97fcf3ad added AddRef to the StartTransientUnit call in machine_start_scope()/manager_start_scope() and a corresponding Unref call in machine_stop_scope(). But when we are running systemd-nspawn@ with --keep unit, the unit is not created by machined so the AddRef never happens. Then when trying to stop the unit, we'd get: systemd-machined[1101]: Sent message type=method_call sender=n/a destination=org.freedesktop.systemd1 path=/org/freedesktop/systemd1 interface=org.freedesktop.systemd1.Manager member=UnrefUnit cookie=37 reply_cookie=0 signature=s error-name=n/a error-message=n/a systemd-machined[1101]: Got message type=error sender=:1.1 destination=:1.13 path=n/a interface=n/a member=n/a cookie=2443 reply_cookie=37 signature=s error-name=org.freedesktop.systemd1.NotReferenced error-message=Unit has not been referenced yet. systemd-machined[1101]: Failed to drop reference to machine scope, ignoring: Unit has not been referenced yet. --- src/machine/machine.c | 10 +++++++--- src/machine/machine.h | 1 + 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/machine/machine.c b/src/machine/machine.c index b5d017a6717..c6475a5398b 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -355,6 +355,7 @@ static int machine_start_scope(Machine *m, sd_bus_message *properties, sd_bus_er return log_error_errno(r, "Failed to start machine scope: %s", bus_error_message(error, r)); m->unit = TAKE_PTR(scope); + m->referenced = true; free_and_replace(m->scope_job, job); } @@ -422,9 +423,12 @@ static int machine_stop_scope(Machine *m) { } else free_and_replace(m->scope_job, job); - q = manager_unref_unit(m->manager, m->unit, &error); - if (q < 0) - log_warning_errno(q, "Failed to drop reference to machine scope, ignoring: %s", bus_error_message(&error, r)); + if (m->referenced) { + q = manager_unref_unit(m->manager, m->unit, &error); + if (q < 0) + log_warning_errno(q, "Failed to drop reference to machine scope, ignoring: %s", bus_error_message(&error, r)); + m->referenced = false; + } return r; } diff --git a/src/machine/machine.h b/src/machine/machine.h index f7471be8f52..0a39e610529 100644 --- a/src/machine/machine.h +++ b/src/machine/machine.h @@ -54,6 +54,7 @@ struct Machine { bool in_gc_queue:1; bool started:1; bool stopping:1; + bool referenced:1; sd_bus_message *create_message;