From 502096b56593919fc947415f6e32bcb680728dac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 8 Feb 2023 09:40:24 +0100 Subject: [PATCH 1/5] core/service: constify ExecCommand* in two functions --- src/core/service.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 9c2fb2ee1c..7513972fb3 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2691,25 +2691,24 @@ _pure_ static bool service_can_reload(Unit *u) { s->type == SERVICE_NOTIFY_RELOAD; } -static unsigned service_exec_command_index(Unit *u, ServiceExecCommand id, ExecCommand *current) { +static unsigned service_exec_command_index(Unit *u, ServiceExecCommand id, const ExecCommand *current) { Service *s = SERVICE(u); unsigned idx = 0; - ExecCommand *first, *c; assert(s); assert(id >= 0); assert(id < _SERVICE_EXEC_COMMAND_MAX); - first = s->exec_command[id]; + const ExecCommand *first = s->exec_command[id]; /* Figure out where we are in the list by walking back to the beginning */ - for (c = current; c != first; c = c->command_prev) + for (const ExecCommand *c = current; c != first; c = c->command_prev) idx++; return idx; } -static int service_serialize_exec_command(Unit *u, FILE *f, ExecCommand *command) { +static int service_serialize_exec_command(Unit *u, FILE *f, const ExecCommand *command) { _cleanup_free_ char *args = NULL, *p = NULL; Service *s = SERVICE(u); const char *type, *key; From 8eb491f4993c6080e9724c0359a87c64c460605e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 8 Feb 2023 10:54:49 +0100 Subject: [PATCH 2/5] test-parse-util: add tests with explicit plus character I expected this to work, but our tests did not cover this explicitly. --- src/test/test-parse-util.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/test/test-parse-util.c b/src/test/test-parse-util.c index 388d0fe3f7..c83e8fc630 100644 --- a/src/test/test-parse-util.c +++ b/src/test/test-parse-util.c @@ -480,6 +480,14 @@ TEST(safe_atou16) { assert_se(r == 0); assert_se(l == 12345); + r = safe_atou16("+12345", &l); + assert_se(r == 0); + assert_se(l == 12345); + + r = safe_atou16(" +12345", &l); + assert_se(r == 0); + assert_se(l == 12345); + r = safe_atou16("123456", &l); assert_se(r == -ERANGE); @@ -514,6 +522,14 @@ TEST(safe_atoi16) { assert_se(r == 0); assert_se(l == -12345); + r = safe_atoi16("+12345", &l); + assert_se(r == 0); + assert_se(l == 12345); + + r = safe_atoi16(" +12345", &l); + assert_se(r == 0); + assert_se(l == 12345); + r = safe_atoi16("32767", &l); assert_se(r == 0); assert_se(l == 32767); @@ -703,6 +719,22 @@ TEST(safe_atoux64) { assert_se(r == 0); assert_se(l == 11603985); + r = safe_atoux64("+12345", &l); + assert_se(r == 0); + assert_se(l == 0x12345); + + r = safe_atoux64(" +12345", &l); + assert_se(r == 0); + assert_se(l == 0x12345); + + r = safe_atoux64("+0x12345", &l); + assert_se(r == 0); + assert_se(l == 0x12345); + + r = safe_atoux64("+0b11011", &l); + assert_se(r == 0); + assert_se(l == 11603985); + r = safe_atoux64("0o11011", &l); assert_se(r == -EINVAL); From a99bd455b59b7922a1b1af480b209263a4d3c659 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 8 Feb 2023 11:30:13 +0100 Subject: [PATCH 3/5] manager: "downgrade" message about command vanishing from the unit file We would print "Current command vanished from the unit file, execution of the command list won't be resumed." as a warning, but most of the time there is nothing to resume, because a unit has just one command. So let's detect the case where the command that was active is the last command in the sequence and skip the warning. I was considering how to store the information that the command is last. An important consideration is not to use a format that would confuse older versions of systemd. (It wouldn't be a big problem if older systemd just refused the new serialization, since we require systemd to be newer, but we should avoid the case where the deserialization is "successful", but actually incorrect.) Similarly, the deserialization from the old systemd must not confuse new systemd. For this command, we have a list of arguments at the end, so just adding a new field either in the middle or at the end is problematic because it's hard to ensure that we don't mix up the positional and variable arguments. We actually need to store just one bit of information, so '+' is prefixed on the index of the last command and used by new systemd to skip the warning. When deserializing from older systemd, '+' is not present, so we detect all commands as "not last", and still emit the warning, so we err on the side of caution. If the user were to deserialize from newer to older systemd, nothing untoward would happen, because the '+' is ignored. (Users shouldn't do this, but we know that this occasionally happens with initrds or exitrds and package downgrades.) --- src/core/service.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 7513972fb3..9ad3c3d995 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2763,7 +2763,16 @@ static int service_serialize_exec_command(Unit *u, FILE *f, const ExecCommand *c return log_oom(); key = strjoina(type, "-command"); - (void) serialize_item_format(f, key, "%s %u %s %s", service_exec_command_to_string(id), idx, p, args); + + /* We use '+1234' instead of '1234' to mark the last command in a sequence. + * This is used in service_deserialize_exec_command(). */ + (void) serialize_item_format( + f, key, + "%s %s%u %s %s", + service_exec_command_to_string(id), + command->command_next ? "" : "+", + idx, + p, args); return 0; } @@ -2877,7 +2886,7 @@ int service_deserialize_exec_command( Service *s = SERVICE(u); int r; unsigned idx = 0, i; - bool control, found = false; + bool control, found = false, last = false; ServiceExecCommand id = _SERVICE_EXEC_COMMAND_INVALID; ExecCommand *command = NULL; _cleanup_free_ char *path = NULL; @@ -2918,9 +2927,15 @@ int service_deserialize_exec_command( state = STATE_EXEC_COMMAND_INDEX; break; case STATE_EXEC_COMMAND_INDEX: + /* PID 1234 is serialized as either '1234' or '+1234'. The second form is used to + * mark the last command in a sequence. We warn if the deserialized command doesn't + * match what we have loaded from the unit, but we don't need to warn if that is the + * last command. */ + r = safe_atou(arg, &idx); if (r < 0) return r; + last = arg[0] == '+'; state = STATE_EXEC_COMMAND_PATH; break; @@ -2965,6 +2980,8 @@ int service_deserialize_exec_command( s->control_command_id = id; } else if (command) s->main_command = command; + else if (last) + log_unit_debug(u, "Current command vanished from the unit file."); else log_unit_warning(u, "Current command vanished from the unit file, execution of the command list won't be resumed."); From 1980a25dc03aa500d4ee2725d696f68d265cd4ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 8 Feb 2023 11:36:22 +0100 Subject: [PATCH 4/5] manager: improve message about Reload/Reexec requests If we fail to get the necessary information, let's just not print that part of the message. 'n/a' looks pretty ugly. I used a bunch of ternary operators instead of seperate log lines because with two components that might or might not be there, we need four different combinations. Also, the unit name doesn't need to be quoted, it's always printable. --- src/core/dbus-manager.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 53121fa1a6..c4f205bc42 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1527,8 +1527,10 @@ static void log_caller(sd_bus_message *message, Manager *manager, const char *me (void) sd_bus_creds_get_comm(creds, &comm); caller = manager_get_unit_by_pid(manager, pid); - log_info("%s requested from client PID " PID_FMT " ('%s') (from unit '%s')...", - method, pid, strna(comm), strna(caller ? caller->id : NULL)); + log_info("%s requested from client PID " PID_FMT "%s%s%s%s%s%s...", + method, pid, + comm ? " ('" : "", strempty(comm), comm ? "')" : "", + caller ? " (unit " : "", caller ? caller->id : NULL, caller ? ")" : ""); } static int method_reload(sd_bus_message *message, void *userdata, sd_bus_error *error) { From f031e8b8dd766533ff1b547759f4328e9da9ae16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 8 Feb 2023 11:58:23 +0100 Subject: [PATCH 5/5] sd-bus: adjust line breaks --- src/libsystemd/sd-bus/sd-bus.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index 10796e25c7..f6a5e4aa06 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -48,7 +48,9 @@ #define log_debug_bus_message(m) \ do { \ sd_bus_message *_mm = (m); \ - log_debug("Got message type=%s sender=%s destination=%s path=%s interface=%s member=%s cookie=%" PRIu64 " reply_cookie=%" PRIu64 " signature=%s error-name=%s error-message=%s", \ + log_debug("Got message type=%s sender=%s destination=%s path=%s interface=%s member=%s " \ + " cookie=%" PRIu64 " reply_cookie=%" PRIu64 \ + " signature=%s error-name=%s error-message=%s", \ strna(bus_message_type_to_string(_mm->header->type)), \ strna(sd_bus_message_get_sender(_mm)), \ strna(sd_bus_message_get_destination(_mm)), \ @@ -1983,7 +1985,9 @@ static int bus_write_message(sd_bus *bus, sd_bus_message *m, size_t *idx) { return r; if (*idx >= BUS_MESSAGE_SIZE(m)) - log_debug("Sent message type=%s sender=%s destination=%s path=%s interface=%s member=%s cookie=%" PRIu64 " reply_cookie=%" PRIu64 " signature=%s error-name=%s error-message=%s", + log_debug("Sent message type=%s sender=%s destination=%s path=%s interface=%s member=%s" + " cookie=%" PRIu64 " reply_cookie=%" PRIu64 + " signature=%s error-name=%s error-message=%s", bus_message_type_to_string(m->header->type), strna(sd_bus_message_get_sender(m)), strna(sd_bus_message_get_destination(m)), @@ -2412,7 +2416,8 @@ _public_ int sd_bus_call( return 1; } - return sd_bus_error_set(error, SD_BUS_ERROR_INCONSISTENT_MESSAGE, "Reply message contained file descriptors which I couldn't accept. Sorry."); + return sd_bus_error_set(error, SD_BUS_ERROR_INCONSISTENT_MESSAGE, + "Reply message contained file descriptors which I couldn't accept. Sorry."); } else if (incoming->header->type == SD_BUS_MESSAGE_METHOD_ERROR) return sd_bus_error_copy(error, &incoming->error); @@ -2924,7 +2929,8 @@ static int process_fd_check(sd_bus *bus, sd_bus_message *m) { if (m->header->type != SD_BUS_MESSAGE_METHOD_CALL) return 1; /* just eat it up */ - return sd_bus_reply_method_errorf(m, SD_BUS_ERROR_INCONSISTENT_MESSAGE, "Message contains file descriptors, which I cannot accept. Sorry."); + return sd_bus_reply_method_errorf(m, SD_BUS_ERROR_INCONSISTENT_MESSAGE, + "Message contains file descriptors, which I cannot accept. Sorry."); } static int process_message(sd_bus *bus, sd_bus_message *m) {