Rework error propagation in systemctl preset (#40504)

User and group systemd-network are created from
sysusers.d/systemd-network.conf, which is only copied into the test
image when building with -Dnetworkd=true. This means that if
-Dnetworkd=false is used, the user and the group don't exist, which
causes the test to fail.

Use a locally created user and group to avoid that.

src/core/manager.c

@@ -1946,11 +1946,8 @@ static void manager_preset_all(Manager *m) {
         log_info("Applying preset policy.");
         r = unit_file_preset_all(RUNTIME_SCOPE_SYSTEM, /* file_flags= */ 0,
                                  /* root_dir= */ NULL, mode, &changes, &n_changes);
-        install_changes_dump(r, "preset", changes, n_changes, /* quiet= */ false);
+        r = install_changes_dump(r, "preset all", changes, n_changes, /* quiet= */ false);
-        if (r < 0)
+        if (r >= 0)
-                log_full_errno(r == -EEXIST ? LOG_NOTICE : LOG_WARNING, r,
-                               "Failed to populate /etc with preset unit settings, ignoring: %m");
-        else
                 log_info("Populated /etc with preset unit settings.");
 }
 

src/shared/install.c

@@ -370,6 +370,24 @@ static void install_change_dump_success(const InstallChange *change) {
         }
 }
 
+/* Generated/transient/missing/invalid units when applying presets.
+ * Coordinate with install_change_dump_error() below. */
+static bool ERRNO_IS_NEG_UNIT_ISSUE(intmax_t r) {
+        return IN_SET(r,
+                      -EEXIST,
+                      -ERFKILL,
+                      -EADDRNOTAVAIL,
+                      -ETXTBSY,
+                      -EBADSLT,
+                      -EIDRM,
+                      -EUCLEAN,
+                      -ELOOP,
+                      -EXDEV,
+                      -ENOENT,
+                      -ENOLINK,
+                      -EUNATCH);
+}
+
 int install_change_dump_error(const InstallChange *change, char **ret_errmsg, const char **ret_bus_error) {
         char *m;
         const char *bus_error;
@@ -462,7 +480,7 @@ int install_change_dump_error(const InstallChange *change, char **ret_errmsg, co
         return 0;
 }
 
-void install_changes_dump(
+int install_changes_dump(
                 int error,
                 const char *verb,
                 const InstallChange *changes,
@@ -476,6 +494,8 @@ void install_changes_dump(
         assert(verb || error >= 0);
         assert(changes || n_changes == 0);
 
+        /* An error is returned if 'error' contains an error or if any of the changes failed. */
+
         FOREACH_ARRAY(i, changes, n_changes)
                 if (i->type >= 0) {
                         if (!quiet)
@@ -487,17 +507,21 @@ void install_changes_dump(
 
                         r = install_change_dump_error(i, &err_message, /* ret_bus_error= */ NULL);
                         if (r == -ENOMEM)
-                                return (void) log_oom();
+                                return log_oom();
                         if (r < 0)
-                                log_error_errno(r, "Failed to %s unit %s: %m", verb, i->path);
+                                RET_GATHER(error,
+                                           log_error_errno(r, "Failed to %s unit %s: %m", verb, i->path));
                         else
-                                log_error_errno(i->type, "Failed to %s unit: %s", verb, err_message);
+                                RET_GATHER(error,
+                                           log_error_errno(i->type, "Failed to %s unit: %s", verb, err_message));
 
                         err_logged = true;
                 }
 
         if (error < 0 && !err_logged)
-                log_error_errno(error, "Failed to %s unit: %m.", verb);
+                log_error_errno(error, "Failed to %s units: %m.", verb);
+
+        return error;
 }
 
 /**
@@ -3609,6 +3633,7 @@ static int preset_prepare_one(
                                   &info, changes, n_changes);
         if (r < 0)
                 return r;
+
         if (!streq(name, info->name)) {
                 log_debug("Skipping %s because it is an alias for %s.", name, info->name);
                 return 0;
@@ -3673,7 +3698,7 @@ int unit_file_preset(
 
         STRV_FOREACH(name, names) {
                 r = preset_prepare_one(scope, &plus, &minus, &lp, *name, &presets, changes, n_changes);
-                if (r < 0)
+                if (r < 0 && !ERRNO_IS_NEG_UNIT_ISSUE(r))
                         return r;
         }
 
@@ -3710,19 +3735,19 @@ int unit_file_preset_all(
         if (r < 0)
                 return r;
 
-        r = 0;
         STRV_FOREACH(i, lp.search_path) {
                 _cleanup_closedir_ DIR *d = NULL;
 
                 d = opendir(*i);
                 if (!d) {
-                        if (errno != ENOENT)
+                        if (errno == ENOENT)
-                                RET_GATHER(r, -errno);
                                 continue;
+
+                        return log_debug_errno(errno, "Failed to opendir %s: %m", *i);
                 }
 
-                FOREACH_DIRENT(de, d, RET_GATHER(r, -errno)) {
+                FOREACH_DIRENT(de, d,
-                        int k;
+                               return log_debug_errno(errno, "Failed to read directory %s: %m", *i)) {
 
                         if (!unit_name_is_valid(de->d_name, UNIT_NAME_ANY))
                                 continue;
@@ -3730,23 +3755,9 @@ int unit_file_preset_all(
                         if (!IN_SET(de->d_type, DT_LNK, DT_REG))
                                 continue;
 
-                        k = preset_prepare_one(scope, &plus, &minus, &lp, de->d_name, &presets, changes, n_changes);
+                        r = preset_prepare_one(scope, &plus, &minus, &lp, de->d_name, &presets, changes, n_changes);
-                        if (k < 0 &&
+                        if (r < 0 && !ERRNO_IS_NEG_UNIT_ISSUE(r))
-                            !IN_SET(k, -EEXIST,
+                                return r;
-                                       -ERFKILL,
-                                       -EADDRNOTAVAIL,
-                                       -ETXTBSY,
-                                       -EBADSLT,
-                                       -EIDRM,
-                                       -EUCLEAN,
-                                       -ELOOP,
-                                       -EXDEV,
-                                       -ENOENT,
-                                       -ENOLINK,
-                                       -EUNATCH))
-                                /* Ignore generated/transient/missing/invalid units when applying preset, propagate other errors.
-                                 * Coordinate with install_change_dump_error() above. */
-                                RET_GATHER(r, k);
                 }
         }
 

src/shared/install.h

@@ -192,7 +192,7 @@ InstallChangeType install_changes_add(InstallChange **changes, size_t *n_changes
 void install_changes_free(InstallChange *changes, size_t n_changes);
 
 int install_change_dump_error(const InstallChange *change, char **ret_errmsg, const char **ret_bus_error);
-void install_changes_dump(
+int install_changes_dump(
                 int error,
                 const char *verb,
                 const InstallChange *changes,

src/systemctl/systemctl-add-dependency.c

@@ -51,7 +51,7 @@ int verb_add_dependency(int argc, char *argv[], void *userdata) {
                 CLEANUP_ARRAY(changes, n_changes, install_changes_free);
 
                 r = unit_file_add_dependency(arg_runtime_scope, unit_file_flags_from_args(), arg_root, names, target, dep, &changes, &n_changes);
-                install_changes_dump(r, "add dependency on", changes, n_changes, arg_quiet);
+                r = install_changes_dump(r, "add dependency on", changes, n_changes, arg_quiet);
                 if (r < 0)
                         return r;
         } else {

src/systemctl/systemctl-enable.c

@@ -258,7 +258,7 @@ int verb_enable(int argc, char *argv[], void *userdata) {
                 else
                         assert_not_reached();
 
-                install_changes_dump(r, verb, changes, n_changes, arg_quiet);
+                r = install_changes_dump(r, verb, changes, n_changes, arg_quiet);
                 if (r < 0)
                         return r;
         }

src/systemctl/systemctl-preset-all.c

@@ -26,7 +26,8 @@ int verb_preset_all(int argc, char *argv[], void *userdata) {
                 CLEANUP_ARRAY(changes, n_changes, install_changes_free);
 
                 r = unit_file_preset_all(arg_runtime_scope, unit_file_flags_from_args(), arg_root, arg_preset_mode, &changes, &n_changes);
-                install_changes_dump(r, "preset", changes, n_changes, arg_quiet);
+                /* We do not treat propagate failure of individual units here. */
+                (void) install_changes_dump(r, "preset all", changes, n_changes, arg_quiet);
                 if (r < 0)
                         return r;
         } else {

src/systemctl/systemctl-set-default.c

@@ -123,7 +123,7 @@ int verb_set_default(int argc, char *argv[], void *userdata) {
                 CLEANUP_ARRAY(changes, n_changes, install_changes_free);
 
                 r = unit_file_set_default(arg_runtime_scope, UNIT_FILE_FORCE, arg_root, unit, &changes, &n_changes);
-                install_changes_dump(r, "set default", changes, n_changes, arg_quiet);
+                r = install_changes_dump(r, "set default", changes, n_changes, arg_quiet);
                 if (r < 0)
                         return r;
         } else {

test/units/TEST-74-AUX-UTILS.userdbctl.sh

@@ -6,6 +6,18 @@ set -o pipefail
 # shellcheck source=test/units/util.sh
 . "$(dirname "$0")"/util.sh
 
+cleanup() {
+    set +e
+    userdel -r test-74-userdbctl
+    groupdel test-74-userdbctl
+}
+
+trap cleanup EXIT
+
+systemd-sysusers - <<EOF
+u test-74-userdbctl - "Test user for TEST-74-AUX-UTILS.userdbctl.sh" / /bin/bash
+EOF
+
 # Root
 userdbctl user root
 userdbctl user 0
@@ -40,11 +52,11 @@ assert_eq "$(userdbctl user 2147418110 -j | jq -r .userName)" foreign-65534
 
 # Make sure that -F shows same data as if we'd ask directly
 userdbctl user root -j | userdbctl -F- user  | cmp - <(userdbctl user root)
-userdbctl user systemd-network -j | userdbctl -F- user  | cmp - <(userdbctl user systemd-network)
+userdbctl user test-74-userdbctl -j | userdbctl -F- user  | cmp - <(userdbctl user test-74-userdbctl)
 userdbctl user 65534 -j | userdbctl -F- user  | cmp - <(userdbctl user 65534)
 
 userdbctl group root -j | userdbctl -F- group  | cmp - <(userdbctl group root)
-userdbctl group systemd-network -j | userdbctl -F- group  | cmp - <(userdbctl group systemd-network)
+userdbctl group test-74-userdbctl -j | userdbctl -F- group  | cmp - <(userdbctl group test-74-userdbctl)
 userdbctl group 65534 -j | userdbctl -F- group  | cmp - <(userdbctl group 65534)
 
 # Ensure NSS doesn't try to automount via open_tree