Merge pull request #27128 from keszybz/sd-bus-docs-and-error-messages

Improvements to man pages for systemd.service, sd-bus, and better error messages

5 files changed, 65 insertions, 68 deletions

diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c
index a6758e616b..0f6e315233 100644
--- a/src/core/dbus-service.c
+++ b/src/core/dbus-service.c
@@ -723,7 +723,7 @@ int bus_service_set_property(
                 return r;
 
         if (u->transient && u->load_state == UNIT_STUB) {
-                /* This is a transient unit, let's load a little more */
+                /* This is a transient unit, let's allow a little more */
 
                 r = bus_service_set_transient_property(s, name, message, flags, error);
                 if (r != 0)
diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c
index a9e63b0678..3f083a8174 100644
--- a/src/core/dbus-unit.c
+++ b/src/core/dbus-unit.c
@@ -2407,14 +2407,13 @@ int bus_unit_set_properties(
         assert(u);
         assert(message);
 
-        /* We iterate through the array twice. First run we just check
-         * if all passed data is valid, second run actually applies
-         * it. This is to implement transaction-like behaviour without
-         * actually providing full transactions. */
+        /* We iterate through the array twice. First run just checks if all passed data is valid, second run
+         * actually applies it. This implements transaction-like behaviour without actually providing full
+         * transactions. */
 
         r = sd_bus_message_enter_container(message, 'a', "(sv)");
         if (r < 0)
-                return r;
+                goto error;
 
         for (;;) {
                 const char *name;
@@ -2422,7 +2421,7 @@ int bus_unit_set_properties(
 
                 r = sd_bus_message_enter_container(message, 'r', "sv");
                 if (r < 0)
-                        return r;
+                        goto error;
                 if (r == 0) {
                         if (for_real || UNIT_WRITE_FLAGS_NOOP(flags))
                                 break;
@@ -2430,7 +2429,7 @@ int bus_unit_set_properties(
                         /* Reached EOF. Let's try again, and this time for realz... */
                         r = sd_bus_message_rewind(message, false);
                         if (r < 0)
-                                return r;
+                                goto error;
 
                         for_real = true;
                         continue;
@@ -2438,11 +2437,11 @@ int bus_unit_set_properties(
 
                 r = sd_bus_message_read(message, "s", &name);
                 if (r < 0)
-                        return r;
+                        goto error;
 
                 r = sd_bus_message_enter_container(message, 'v', NULL);
                 if (r < 0)
-                        return r;
+                        goto error;
 
                 /* If not for real, then mask out the two target flags */
                 f = for_real ? flags : (flags & ~(UNIT_RUNTIME|UNIT_PERSISTENT));
@@ -2456,7 +2455,7 @@ int bus_unit_set_properties(
                 if (r == 0)
                         r = bus_unit_set_live_property(u, name, message, f, error);
                 if (r < 0)
-                        return r;
+                        goto error;
 
                 if (r == 0)
                         return sd_bus_error_setf(error, SD_BUS_ERROR_PROPERTY_READ_ONLY,
@@ -2464,23 +2463,32 @@ int bus_unit_set_properties(
 
                 r = sd_bus_message_exit_container(message);
                 if (r < 0)
-                        return r;
+                        goto error;
 
                 r = sd_bus_message_exit_container(message);
                 if (r < 0)
-                        return r;
+                        goto error;
 
                 n += for_real;
         }
 
         r = sd_bus_message_exit_container(message);
         if (r < 0)
-                return r;
+                goto error;
 
         if (commit && n > 0 && UNIT_VTABLE(u)->bus_commit_properties)
                 UNIT_VTABLE(u)->bus_commit_properties(u);
 
         return n;
+
+ error:
+        /* Pretty much any of the calls above can fail if the message is not formed properly
+         * or if it has unexpected contents. Fill in a more informative error message here. */
+        if (sd_bus_error_is_set(error))
+                return r;
+        return sd_bus_error_set_errnof(error, r,
+                                       r == -ENXIO ? "Failed to set unit properties: Unexpected message contents"
+                                                   : "Failed to set unit properties: %m");
 }
 
 int bus_unit_validate_load_state(Unit *u, sd_bus_error *error) {
diff --git a/src/core/execute.c b/src/core/execute.c
index 81e48f12c6..b1160cd4d6 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -4397,6 +4397,22 @@ static int collect_open_file_fds(
         return 0;
 }
 
+static void log_command_line(Unit *unit, const char *msg, const char *executable, char **argv) {
+        assert(unit);
+        assert(msg);
+        assert(executable);
+
+        if (!DEBUG_LOGGING)
+                return;
+
+        _cleanup_free_ char *cmdline = quote_command_line(argv, SHELL_ESCAPE_EMPTY);
+
+        log_unit_struct(unit, LOG_DEBUG,
+                        "EXECUTABLE=%s", executable,
+                        LOG_UNIT_MESSAGE(unit, "%s: %s", msg, strnull(cmdline)),
+                        LOG_UNIT_INVOCATION_ID(unit));
+}
+
 static int exec_child(
                 Unit *unit,
                 const ExecCommand *command,
@@ -4655,8 +4671,7 @@ static int exec_child(
                 return log_unit_error_errno(unit, r, "Failed to determine $HOME for user: %m");
         }
 
-        /* If a socket is connected to STDIN/STDOUT/STDERR, we
-         * must sure to drop O_NONBLOCK */
+        /* If a socket is connected to STDIN/STDOUT/STDERR, we must drop O_NONBLOCK */
         if (socket_fd >= 0)
                 (void) fd_nonblock(socket_fd, false);
 
@@ -5197,9 +5212,10 @@ static int exec_child(
         }
 #endif
 
-        /* We repeat the fd closing here, to make sure that nothing is leaked from the PAM modules. Note that we are
-         * more aggressive this time since socket_fd and the netns and ipcns fds we don't need anymore. We do keep the exec_fd
-         * however if we have it as we want to keep it open until the final execve(). */
+        /* We repeat the fd closing here, to make sure that nothing is leaked from the PAM modules. Note that
+         * we are more aggressive this time, since we don't need socket_fd and the netns and ipcns fds any
+         * more. We do keep exec_fd however, if we have it, since we need to keep it open until the final
+         * execve(). */
 
         r = close_all_fds(keep_fds, n_keep_fds);
         if (r >= 0)
@@ -5221,9 +5237,9 @@ static int exec_child(
         if (needs_sandboxing) {
                 uint64_t bset;
 
-                /* Set the RTPRIO resource limit to 0, but only if nothing else was explicitly
-                 * requested. (Note this is placed after the general resource limit initialization, see
-                 * above, in order to take precedence.) */
+                /* Set the RTPRIO resource limit to 0, but only if nothing else was explicitly requested.
+                 * (Note this is placed after the general resource limit initialization, see above, in order
+                 * to take precedence.) */
                 if (context->restrict_realtime && !context->rlimit[RLIMIT_RTPRIO]) {
                         if (setrlimit(RLIMIT_RTPRIO, &RLIMIT_MAKE_CONST(0)) < 0) {
                                 *exit_status = EXIT_LIMITS;
@@ -5496,19 +5512,7 @@ static int exec_child(
         } else
                 final_argv = command->argv;
 
-        if (DEBUG_LOGGING) {
-                _cleanup_free_ char *line = NULL;
-
-                line = quote_command_line(final_argv, SHELL_ESCAPE_EMPTY);
-                if (!line) {
-                        *exit_status = EXIT_MEMORY;
-                        return log_oom();
-                }
-
-                log_unit_struct(unit, LOG_DEBUG,
-                                "EXECUTABLE=%s", executable,
-                                LOG_UNIT_MESSAGE(unit, "Executing: %s", line));
-        }
+        log_command_line(unit, "Executing", executable, final_argv);
 
         if (exec_fd >= 0) {
                 uint8_t hot = 1;
@@ -5555,7 +5559,6 @@ int exec_spawn(Unit *unit,
         _cleanup_free_ char *subcgroup_path = NULL;
         _cleanup_strv_free_ char **files_env = NULL;
         size_t n_storage_fds = 0, n_socket_fds = 0;
-        _cleanup_free_ char *line = NULL;
         pid_t pid;
 
         assert(unit);
@@ -5593,21 +5596,13 @@ int exec_spawn(Unit *unit,
         if (r < 0)
                 return log_unit_error_errno(unit, r, "Failed to load environment files: %m");
 
-        line = quote_command_line(command->argv, SHELL_ESCAPE_EMPTY);
-        if (!line)
-                return log_oom();
-
         /* Fork with up-to-date SELinux label database, so the child inherits the up-to-date db
            and, until the next SELinux policy changes, we save further reloads in future children. */
         mac_selinux_maybe_reload();
 
-        log_unit_struct(unit, LOG_DEBUG,
-                        LOG_UNIT_MESSAGE(unit, "About to execute %s", line),
-                        "EXECUTABLE=%s", command->path, /* We won't know the real executable path until we create
-                                                           the mount namespace in the child, but we want to log
-                                                           from the parent, so we need to use the (possibly
-                                                           inaccurate) path here. */
-                        LOG_UNIT_INVOCATION_ID(unit));
+        /* We won't know the real executable path until we create the mount namespace in the child, but we
+           want to log from the parent, so we use the possibly inaccurate path here. */
+        log_command_line(unit, "About to execute", command->path, command->argv);
 
         if (params->cgroup_path) {
                 r = exec_parameters_get_cgroup_path(params, &subcgroup_path);
@@ -6895,7 +6890,7 @@ void exec_command_append_list(ExecCommand **l, ExecCommand *e) {
                 end = LIST_FIND_TAIL(command, *l);
                 LIST_INSERT_AFTER(command, *l, end, e);
         } else
-              *l = e;
+                *l = e;
 }
 
 int exec_command_set(ExecCommand *c, const char *path, ...) {
diff --git a/src/core/unit.c b/src/core/unit.c
index 642db41e41..846d15b415 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -3465,7 +3465,7 @@ static int get_name_owner_handler(sd_bus_message *message, void *userdata, sd_bu
 
         e = sd_bus_message_get_error(message);
         if (e) {
-                if (!sd_bus_error_has_name(e, "org.freedesktop.DBus.Error.NameHasNoOwner")) {
+                if (!sd_bus_error_has_name(e, SD_BUS_ERROR_NAME_HAS_NO_OWNER)) {
                         r = sd_bus_error_get_errno(e);
                         log_unit_error_errno(u, r,
                                              "Unexpected error response from GetNameOwner(): %s",
@@ -4310,20 +4310,18 @@ static const char* unit_drop_in_dir(Unit *u, UnitWriteFlags flags) {
         return NULL;
 }
 
-char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) {
+const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) {
+        assert(s);
         assert(!FLAGS_SET(flags, UNIT_ESCAPE_EXEC_SYNTAX | UNIT_ESCAPE_C));
+        assert(buf);
 
         _cleanup_free_ char *t = NULL;
 
-        if (!s)
-                return NULL;
-
-        /* Escapes the input string as requested. Returns the escaped string. If 'buf' is specified then the
-         * allocated return buffer pointer is also written to *buf, except if no escaping was necessary, in
-         * which case *buf is set to NULL, and the input pointer is returned as-is. This means the return
-         * value always contains a properly escaped version, but *buf when passed only contains a pointer if
-         * an allocation was necessary. If *buf is not specified, then the return value always needs to be
-         * freed. Callers can use this to optimize memory allocations. */
+        /* Returns a string with any escaping done. If no escaping was necessary, *buf is set to NULL, and
+         * the input pointer is returned as-is. If an allocation was needed, the return buffer pointer is
+         * written to *buf. This means the return value always contains a properly escaped version, but *buf
+         * only contains a pointer if an allocation was made. Callers can use this to optimize memory
+         * allocations. */
 
         if (flags & UNIT_ESCAPE_SPECIFIERS) {
                 t = specifier_escape(s);
@@ -4333,8 +4331,8 @@ char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) {
                 s = t;
         }
 
-        /* We either do c-escaping or shell-escaping, to additionally escape characters that we parse for
-         * ExecStart= and friend, i.e. '$' and ';' and quotes. */
+        /* We either do C-escaping or shell-escaping, to additionally escape characters that we parse for
+         * ExecStart= and friends, i.e. '$' and ';' and quotes. */
 
         if (flags & UNIT_ESCAPE_EXEC_SYNTAX) {
                 char *t2 = shell_escape(s, "$;'\"");
@@ -4353,12 +4351,8 @@ char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) {
                 s = t;
         }
 
-        if (buf) {
-                *buf = TAKE_PTR(t);
-                return (char*) s;
-        }
-
-        return TAKE_PTR(t) ?: strdup(s);
+        *buf = TAKE_PTR(t);
+        return s;
 }
 
 char* unit_concat_strv(char **l, UnitWriteFlags flags) {
diff --git a/src/core/unit.h b/src/core/unit.h
index 513c8181f5..420405b2b7 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -964,7 +964,7 @@ ExecRuntime *unit_get_exec_runtime(Unit *u) _pure_;
 
 int unit_setup_exec_runtime(Unit *u);
 
-char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf);
+const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf);
 char* unit_concat_strv(char **l, UnitWriteFlags flags);
 
 int unit_write_setting(Unit *u, UnitWriteFlags flags, const char *name, const char *data);