systemctl: fix silent failure when --root is not found

Some calls to lookup_path_init() were not followed by any log emission.
E.g.:
$ SYSTEMD_LOG_LEVEL=debug systemctl --root=/missing enable unit; echo $?
1

Let's add a helper function and use it in various places.

$ SYSTEMD_LOG_LEVEL=debug build/systemctl --root=/missing enable unit; echo $?
Failed to initialize unit search paths for root directory /missing: No such file or directory
1
$ SYSTEMCTL_SKIP_SYSV=1 build/systemctl --root=/missing enable unit; echo $?
Failed to initialize unit search paths for root directory /missing: No such file or directory
Failed to enable: No such file or directory.
1

The repeated error in the second case is not very nice, but this is a niche
case and I don't think it's worth the trouble to trying to avoid it.

diff --git a/src/analyze/analyze-unit-files.c b/src/analyze/analyze-unit-files.c
index 60fbdcb2306e88314618e016d27aff636912333e..f07ec3e56cd79b8735e3904a9bbbfaacb21f982c 100644 (file)
--- a/src/analyze/analyze-unit-files.c
+++ b/src/analyze/analyze-unit-files.c
@@ -21,9 +21,9 @@ int verb_unit_files(int argc, char *argv[], void *userdata) {
         char **v;
         int r;
 
-        r = lookup_paths_init(&lp, arg_scope, 0, NULL);
+        r = lookup_paths_init_or_warn(&lp, arg_scope, 0, NULL);
         if (r < 0)
-                return log_error_errno(r, "lookup_paths_init() failed: %m");
+                return r;
 
         r = unit_file_build_name_map(&lp, NULL, &unit_ids, &unit_names, NULL);
         if (r < 0)

diff --git a/src/analyze/analyze-unit-paths.c b/src/analyze/analyze-unit-paths.c
index 32c97b2e520dd7d3f7cb0b4ff4c98a460ee2e132..6fa1527dd6f46ccf9990514c425fed05d74c192b 100644 (file)
--- a/src/analyze/analyze-unit-paths.c
+++ b/src/analyze/analyze-unit-paths.c
@@ -9,9 +9,9 @@ int verb_unit_paths(int argc, char *argv[], void *userdata) {
         _cleanup_(lookup_paths_free) LookupPaths paths = {};
         int r;
 
-        r = lookup_paths_init(&paths, arg_scope, 0, NULL);
+        r = lookup_paths_init_or_warn(&paths, arg_scope, 0, NULL);
         if (r < 0)
-                return log_error_errno(r, "lookup_paths_init() failed: %m");
+                return r;
 
         STRV_FOREACH(p, paths.search_path)
                 puts(*p);

diff --git a/src/basic/env-file.c b/src/basic/env-file.c
index 14925e72e195de50972f54f33c112abcfb0786d5..2ab5c7bc0df3016d29acebf49232afed5615f4f4 100644 (file)
--- a/src/basic/env-file.c
+++ b/src/basic/env-file.c
@@ -16,9 +16,8 @@ static int parse_env_file_internal(
                 FILE *f,
                 const char *fname,
                 int (*push) (const char *filename, unsigned line,
-                             const char *key, char *value, void *userdata, int *n_pushed),
-                void *userdata,
-                int *n_pushed) {
+                             const char *key, char *value, void *userdata),
+                void *userdata) {
 
         size_t n_key = 0, n_value = 0, last_value_whitespace = SIZE_MAX, last_key_whitespace = SIZE_MAX;
         _cleanup_free_ char *contents = NULL, *key = NULL, *value = NULL;
@@ -99,7 +98,7 @@ static int parse_env_file_internal(
                                 if (last_key_whitespace != SIZE_MAX)
                                         key[last_key_whitespace] = 0;
 
-                                r = push(fname, line, key, value, userdata, n_pushed);
+                                r = push(fname, line, key, value, userdata);
                                 if (r < 0)
                                         return r;
 
@@ -142,7 +141,7 @@ static int parse_env_file_internal(
                                 if (last_key_whitespace != SIZE_MAX)
                                         key[last_key_whitespace] = 0;
 
-                                r = push(fname, line, key, value, userdata, n_pushed);
+                                r = push(fname, line, key, value, userdata);
                                 if (r < 0)
                                         return r;
 
@@ -261,7 +260,7 @@ static int parse_env_file_internal(
                 if (last_key_whitespace != SIZE_MAX)
                         key[last_key_whitespace] = 0;
 
-                r = push(fname, line, key, value, userdata, n_pushed);
+                r = push(fname, line, key, value, userdata);
                 if (r < 0)
                         return r;
 
@@ -299,8 +298,7 @@ static int check_utf8ness_and_warn(
 static int parse_env_file_push(
                 const char *filename, unsigned line,
                 const char *key, char *value,
-                void *userdata,
-                int *n_pushed) {
+                void *userdata) {
 
         const char *k;
         va_list aq, *ap = userdata;
@@ -322,9 +320,6 @@ static int parse_env_file_push(
                         free(*v);
                         *v = value;
 
-                        if (n_pushed)
-                                (*n_pushed)++;
-
                         return 1;
                 }
         }
@@ -340,16 +335,13 @@ int parse_env_filev(
                 const char *fname,
                 va_list ap) {
 
-        int r, n_pushed = 0;
+        int r;
         va_list aq;
 
         va_copy(aq, ap);
-        r = parse_env_file_internal(f, fname, parse_env_file_push, &aq, &n_pushed);
+        r = parse_env_file_internal(f, fname, parse_env_file_push, &aq);
         va_end(aq);
-        if (r < 0)
-                return r;
-
-        return n_pushed;
+        return r;
 }
 
 int parse_env_file_sentinel(
@@ -370,8 +362,7 @@ int parse_env_file_sentinel(
 static int load_env_file_push(
                 const char *filename, unsigned line,
                 const char *key, char *value,
-                void *userdata,
-                int *n_pushed) {
+                void *userdata) {
         char ***m = userdata;
         char *p;
         int r;
@@ -388,34 +379,28 @@ static int load_env_file_push(
         if (r < 0)
                 return r;
 
-        if (n_pushed)
-                (*n_pushed)++;
-
         free(value);
         return 0;
 }
 
 int load_env_file(FILE *f, const char *fname, char ***rl) {
-        char **m = NULL;
+        _cleanup_strv_free_ char **m = NULL;
         int r;
 
-        r = parse_env_file_internal(f, fname, load_env_file_push, &m, NULL);
-        if (r < 0) {
-                strv_free(m);
+        r = parse_env_file_internal(f, fname, load_env_file_push, &m);
+        if (r < 0)
                 return r;
-        }
 
-        *rl = m;
+        *rl = TAKE_PTR(m);
         return 0;
 }
 
 static int load_env_file_push_pairs(
                 const char *filename, unsigned line,
                 const char *key, char *value,
-                void *userdata,
-                int *n_pushed) {
+                void *userdata) {
+
         char ***m = ASSERT_PTR(userdata);
-        bool added = false;
         int r;
 
         r = check_utf8ness_and_warn(filename, line, key, value);
@@ -426,49 +411,37 @@ static int load_env_file_push_pairs(
         for (char **t = *m; t && *t; t += 2)
                 if (streq(t[0], key)) {
                         if (value)
-                                r = free_and_replace(t[1], value);
+                                return free_and_replace(t[1], value);
                         else
-                                r = free_and_strdup(t+1, "");
-                        goto finish;
+                                return free_and_strdup(t+1, "");
                 }
 
         r = strv_extend(m, key);
         if (r < 0)
-                return -ENOMEM;
+                return r;
 
         if (value)
-                r = strv_push(m, value);
+                return strv_push(m, value);
         else
-                r = strv_extend(m, "");
-        added = true;
- finish:
-        if (r < 0)
-                return r;
-
-        if (n_pushed && added)
-                (*n_pushed)++;
-        return 0;
+                return strv_extend(m, "");
 }
 
 int load_env_file_pairs(FILE *f, const char *fname, char ***rl) {
-        char **m = NULL;
+        _cleanup_strv_free_ char **m = NULL;
         int r;
 
-        r = parse_env_file_internal(f, fname, load_env_file_push_pairs, &m, NULL);
-        if (r < 0) {
-                strv_free(m);
+        r = parse_env_file_internal(f, fname, load_env_file_push_pairs, &m);
+        if (r < 0)
                 return r;
-        }
 
-        *rl = m;
+        *rl = TAKE_PTR(m);
         return 0;
 }
 
 static int merge_env_file_push(
                 const char *filename, unsigned line,
                 const char *key, char *value,
-                void *userdata,
-                int *n_pushed) {
+                void *userdata) {
 
         char ***env = userdata;
         char *expanded_value;
@@ -497,7 +470,7 @@ static int merge_env_file_push(
 
         log_debug("%s:%u: setting %s=%s", filename, line, key, value);
 
-        return load_env_file_push(filename, line, key, value, env, n_pushed);
+        return load_env_file_push(filename, line, key, value, env);
 }
 
 int merge_env_file(
@@ -509,7 +482,7 @@ int merge_env_file(
          * plus "extended" substitutions, unlike other exported parsing functions.
          */
 
-        return parse_env_file_internal(f, fname, merge_env_file_push, env, NULL);
+        return parse_env_file_internal(f, fname, merge_env_file_push, env);
 }
 
 static void write_env_var(FILE *f, const char *v) {

diff --git a/src/basic/path-lookup.c b/src/basic/path-lookup.c
index 2dd587fd8a223288d4ff81effda03085adbbc722..b699756658490d8503e33bf6a6a9e25dc2a342c0 100644 (file)
--- a/src/basic/path-lookup.c
+++ b/src/basic/path-lookup.c
@@ -508,7 +508,7 @@ static int get_paths_from_environ(const char *var, char ***paths, bool *append)
 }
 
 int lookup_paths_init(
-                LookupPaths *p,
+                LookupPaths *lp,
                 UnitFileScope scope,
                 LookupPathsFlags flags,
                 const char *root_dir) {
@@ -526,7 +526,7 @@ int lookup_paths_init(
         _cleanup_strv_free_ char **paths = NULL;
         int r;
 
-        assert(p);
+        assert(lp);
         assert(scope >= 0);
         assert(scope < _UNIT_FILE_SCOPE_MAX);
 
@@ -716,7 +716,7 @@ int lookup_paths_init(
         if (r < 0)
                 return -ENOMEM;
 
-        *p = (LookupPaths) {
+        *lp = (LookupPaths) {
                 .search_path = strv_uniq(TAKE_PTR(paths)),
 
                 .persistent_config = TAKE_PTR(persistent_config),
@@ -741,41 +741,51 @@ int lookup_paths_init(
         return 0;
 }
 
-void lookup_paths_free(LookupPaths *p) {
-        if (!p)
+int lookup_paths_init_or_warn(LookupPaths *lp, UnitFileScope scope, LookupPathsFlags flags, const char *root_dir) {
+        int r;
+
+        r = lookup_paths_init(lp, scope, flags, root_dir);
+        if (r < 0)
+                return log_error_errno(r, "Failed to initialize unit search paths%s%s: %m",
+                                       isempty(root_dir) ? "" : " for root directory ", strempty(root_dir));
+        return r;
+}
+
+void lookup_paths_free(LookupPaths *lp) {
+        if (!lp)
                 return;
 
-        p->search_path = strv_free(p->search_path);
+        lp->search_path = strv_free(lp->search_path);
 
-        p->persistent_config = mfree(p->persistent_config);
-        p->runtime_config = mfree(p->runtime_config);
+        lp->persistent_config = mfree(lp->persistent_config);
+        lp->runtime_config = mfree(lp->runtime_config);
 
-        p->persistent_attached = mfree(p->persistent_attached);
-        p->runtime_attached = mfree(p->runtime_attached);
+        lp->persistent_attached = mfree(lp->persistent_attached);
+        lp->runtime_attached = mfree(lp->runtime_attached);
 
-        p->generator = mfree(p->generator);
-        p->generator_early = mfree(p->generator_early);
-        p->generator_late = mfree(p->generator_late);
+        lp->generator = mfree(lp->generator);
+        lp->generator_early = mfree(lp->generator_early);
+        lp->generator_late = mfree(lp->generator_late);
 
-        p->transient = mfree(p->transient);
+        lp->transient = mfree(lp->transient);
 
-        p->persistent_control = mfree(p->persistent_control);
-        p->runtime_control = mfree(p->runtime_control);
+        lp->persistent_control = mfree(lp->persistent_control);
+        lp->runtime_control = mfree(lp->runtime_control);
 
-        p->root_dir = mfree(p->root_dir);
-        p->temporary_dir = mfree(p->temporary_dir);
+        lp->root_dir = mfree(lp->root_dir);
+        lp->temporary_dir = mfree(lp->temporary_dir);
 }
 
-void lookup_paths_log(LookupPaths *p) {
-        assert(p);
+void lookup_paths_log(LookupPaths *lp) {
+        assert(lp);
 
-        if (strv_isempty(p->search_path)) {
+        if (strv_isempty(lp->search_path)) {
                 log_debug("Ignoring unit files.");
-                p->search_path = strv_free(p->search_path);
+                lp->search_path = strv_free(lp->search_path);
         } else {
                 _cleanup_free_ char *t = NULL;
 
-                t = strv_join(p->search_path, "\n\t");
+                t = strv_join(lp->search_path, "\n\t");
                 log_debug("Looking for unit files in (higher priority first):\n\t%s", strna(t));
         }
 }

diff --git a/src/basic/path-lookup.h b/src/basic/path-lookup.h
index af85dc7b4f6cdb21a16ab26dc8488b876a9f85cf..1f0e5ea271f7ae8c0fdde8cad048d9f06c3feaba 100644 (file)
--- a/src/basic/path-lookup.h
+++ b/src/basic/path-lookup.h
@@ -54,7 +54,8 @@ struct LookupPaths {
         char *temporary_dir;
 };
 
-int lookup_paths_init(LookupPaths *p, UnitFileScope scope, LookupPathsFlags flags, const char *root_dir);
+int lookup_paths_init(LookupPaths *lp, UnitFileScope scope, LookupPathsFlags flags, const char *root_dir);
+int lookup_paths_init_or_warn(LookupPaths *lp, UnitFileScope scope, LookupPathsFlags flags, const char *root_dir);
 
 int xdg_user_dirs(char ***ret_config_dirs, char ***ret_data_dirs);
 int xdg_user_runtime_dir(char **ret, const char *suffix);

diff --git a/src/core/manager.c b/src/core/manager.c
index 69717e5ba6e884b9799a97c58d315757480502be..0fbb7d290e1f2204cab831f4428060c08a122fdc 100644 (file)
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -1751,11 +1751,11 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds, const char *roo
 
         /* If we are running in test mode, we still want to run the generators,
          * but we should not touch the real generator directories. */
-        r = lookup_paths_init(&m->lookup_paths, m->unit_file_scope,
-                              MANAGER_IS_TEST_RUN(m) ? LOOKUP_PATHS_TEMPORARY_GENERATED : 0,
-                              root);
+        r = lookup_paths_init_or_warn(&m->lookup_paths, m->unit_file_scope,
+                                      MANAGER_IS_TEST_RUN(m) ? LOOKUP_PATHS_TEMPORARY_GENERATED : 0,
+                                      root);
         if (r < 0)
-                return log_error_errno(r, "Failed to initialize path lookup table: %m");
+                return r;
 
         dual_timestamp_get(m->timestamps + manager_timestamp_initrd_mangle(MANAGER_TIMESTAMP_GENERATORS_START));
         r = manager_run_environment_generators(m);
@@ -3343,9 +3343,9 @@ int manager_reload(Manager *m) {
         m->uid_refs = hashmap_free(m->uid_refs);
         m->gid_refs = hashmap_free(m->gid_refs);
 
-        r = lookup_paths_init(&m->lookup_paths, m->unit_file_scope, 0, NULL);
+        r = lookup_paths_init_or_warn(&m->lookup_paths, m->unit_file_scope, 0, NULL);
         if (r < 0)
-                log_warning_errno(r, "Failed to initialize path lookup table, ignoring: %m");
+                return r;
 
         (void) manager_run_environment_generators(m);
         (void) manager_run_generators(m);

diff --git a/src/shared/condition.c b/src/shared/condition.c
index 0a4072c9bef58bb645657d40df19d1aae9d60aaa..15b22a4006e38e6c7d4d358b2a98e693117ab991 100644 (file)
--- a/src/shared/condition.c
+++ b/src/shared/condition.c
@@ -785,7 +785,8 @@ static int condition_test_needs_update(Condition *c, char **env) {
         if (r < 0) {
                 log_debug_errno(r, "Failed to parse timestamp file '%s', using mtime: %m", p);
                 return true;
-        } else if (r == 0) {
+        }
+        if (isempty(timestamp_str)) {
                 log_debug("No data in timestamp file '%s', using mtime.", p);
                 return true;
         }

diff --git a/src/shared/install.c b/src/shared/install.c
index 554c530cc4b722add797811bbe0d7d19a9d38b1d..cfada1344240f1286b37359991a1b6732976c101 100644 (file)
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -2597,7 +2597,7 @@ int unit_file_enable(
         assert(scope >= 0);
         assert(scope < _UNIT_FILE_SCOPE_MAX);
 
-        r = lookup_paths_init(&lp, scope, 0, root_dir);
+        r = lookup_paths_init_or_warn(&lp, scope, 0, root_dir);
         if (r < 0)
                 return r;
 

diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c
index 6ef6cc54a8a50924621b2bb5c02802435b5a5224..f8f810e1cddaacab28a0393eb678b7a06b1f5d86 100644 (file)
--- a/src/systemctl/systemctl-edit.c
+++ b/src/systemctl/systemctl-edit.c
@@ -37,9 +37,9 @@ int verb_cat(int argc, char *argv[], void *userdata) {
         if (arg_transport != BUS_TRANSPORT_LOCAL)
                 return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot remotely cat units.");
 
-        r = lookup_paths_init(&lp, arg_scope, 0, arg_root);
+        r = lookup_paths_init_or_warn(&lp, arg_scope, 0, arg_root);
         if (r < 0)
-                return log_error_errno(r, "Failed to determine unit paths: %m");
+                return r;
 
         r = acquire_bus(BUS_MANAGER, &bus);
         if (r < 0)
@@ -507,9 +507,9 @@ int verb_edit(int argc, char *argv[], void *userdata) {
         if (arg_transport != BUS_TRANSPORT_LOCAL)
                 return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot edit units remotely.");
 
-        r = lookup_paths_init(&lp, arg_scope, 0, arg_root);
+        r = lookup_paths_init_or_warn(&lp, arg_scope, 0, arg_root);
         if (r < 0)
-                return log_error_errno(r, "Failed to determine unit paths: %m");
+                return r;
 
         r = mac_selinux_init();
         if (r < 0)

diff --git a/src/systemctl/systemctl-enable.c b/src/systemctl/systemctl-enable.c
index b18c516b260967f1f3d9985e0fcf8bb7873784ba..a1ca837d4d74ae51a2685860327ba8cd3e4c06f4 100644 (file)
--- a/src/systemctl/systemctl-enable.c
+++ b/src/systemctl/systemctl-enable.c
@@ -141,7 +141,7 @@ int verb_enable(int argc, char *argv[], void *userdata) {
                 if (STR_IN_SET(verb, "mask", "unmask")) {
                         _cleanup_(lookup_paths_free) LookupPaths lp = {};
 
-                        r = lookup_paths_init(&lp, arg_scope, 0, arg_root);
+                        r = lookup_paths_init_or_warn(&lp, arg_scope, 0, arg_root);
                         if (r < 0)
                                 return r;
 

diff --git a/src/systemctl/systemctl-sysv-compat.c b/src/systemctl/systemctl-sysv-compat.c
index 017dba20340e74e1e5a44ed440aa8fa6ac35a4d0..c6e8defd1b588c2d6c20066092d109bbe399808f 100644 (file)
--- a/src/systemctl/systemctl-sysv-compat.c
+++ b/src/systemctl/systemctl-sysv-compat.c
@@ -128,7 +128,7 @@ int enable_sysv_units(const char *verb, char **args) {
                         "is-enabled"))
                 return 0;
 
-        r = lookup_paths_init(&paths, arg_scope, LOOKUP_PATHS_EXCLUDE_GENERATED, arg_root);
+        r = lookup_paths_init_or_warn(&paths, arg_scope, LOOKUP_PATHS_EXCLUDE_GENERATED, arg_root);
         if (r < 0)
                 return r;
 

diff --git a/src/sysv-generator/sysv-generator.c b/src/sysv-generator/sysv-generator.c
index a81df6600ba843a28f8cebd4e700c15ac843f6f3..428509f4ce71e0baa486702baeb992dc6b0a67b7 100644 (file)
--- a/src/sysv-generator/sysv-generator.c
+++ b/src/sysv-generator/sysv-generator.c
@@ -891,9 +891,9 @@ static int run(const char *dest, const char *dest_early, const char *dest_late)
 
         assert_se(arg_dest = dest_late);
 
-        r = lookup_paths_init(&lp, UNIT_FILE_SYSTEM, LOOKUP_PATHS_EXCLUDE_GENERATED, NULL);
+        r = lookup_paths_init_or_warn(&lp, UNIT_FILE_SYSTEM, LOOKUP_PATHS_EXCLUDE_GENERATED, NULL);
         if (r < 0)
-                return log_error_errno(r, "Failed to find lookup paths: %m");
+                return r;
 
         all_services = hashmap_new(&string_hash_ops);
         if (!all_services)

diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c
index 3e626dbe93cb15d8f0d230fcdc33cf060e4c086c..3e98d94019c97a3ba5c58a6c64cf367307aabcf8 100644 (file)
--- a/src/test/test-fileio.c
+++ b/src/test/test-fileio.c
@@ -109,8 +109,7 @@ TEST(parse_env_file) {
                        "eleven", &eleven,
                        "twelve", &twelve,
                        "thirteen", &thirteen);
-
-        assert_se(r >= 0);
+        assert_se(r == 0);
 
         log_info("one=[%s]", strna(one));
         log_info("two=[%s]", strna(two));

diff --git a/src/test/test-os-util.c b/src/test/test-os-util.c
index d6336c53e9dfaddc1b238fdce7b6204b51c60786..2cee6470c4a2f8236e11c710e9bbba518e634ce9 100644 (file)
--- a/src/test/test-os-util.c
+++ b/src/test/test-os-util.c
@@ -18,7 +18,7 @@ TEST(path_is_os_tree) {
 TEST(parse_os_release) {
         /* Let's assume that we're running in a valid system, so os-release is available */
         _cleanup_free_ char *id = NULL, *id2 = NULL, *name = NULL, *foobar = NULL;
-        assert_se(parse_os_release(NULL, "ID", &id) == 1);
+        assert_se(parse_os_release(NULL, "ID", &id) == 0);
         log_info("ID: %s", id);
 
         assert_se(setenv("SYSTEMD_OS_RELEASE", "/dev/null", 1) == 0);
@@ -31,7 +31,7 @@ TEST(parse_os_release) {
                                 "NAME=the-name") == 0);
 
         assert_se(setenv("SYSTEMD_OS_RELEASE", tmpfile, 1) == 0);
-        assert_se(parse_os_release(NULL, "ID", &id, "NAME", &name) == 2);
+        assert_se(parse_os_release(NULL, "ID", &id, "NAME", &name) == 0);
         log_info("ID: %s NAME: %s", id, name);
         assert_se(streq(id, "the-id"));
         assert_se(streq(name, "the-name"));
@@ -43,8 +43,7 @@ TEST(parse_os_release) {
                                 "NAME='the-name'") == 0);
 
         assert_se(setenv("SYSTEMD_OS_RELEASE", tmpfile2, 1) == 0);
-        // FIXME: we return 3, which means that the return value is useless in face of repeats
-        assert_se(parse_os_release(NULL, "ID", &id, "NAME", &name) == 3);
+        assert_se(parse_os_release(NULL, "ID", &id, "NAME", &name) == 0);
         log_info("ID: %s NAME: %s", id, name);
         assert_se(streq(id, "the-id"));
         assert_se(streq(name, "the-name"));