From 8fa3444f3ca7add9af40ab565e045c2754e5a855 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>

Date: Thu, 10 Mar 2022 16:47:51 +0100

Subject: [PATCH] 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.

(cherry picked from commit 99aad9a2b9e2c06023a2043976fd9395332ff097)

Related: #2082131

---

 src/basic/env-file.c                  | 83 +++++++++------------------

 src/basic/path-lookup.c               | 56 ++++++++++--------

 src/basic/path-lookup.h               |  3 +-

 src/core/manager.c                    | 12 ++--

 src/shared/condition.c                |  3 +-

 src/shared/install.c                  |  2 +-

 src/systemctl/systemctl-edit.c        |  8 +--

 src/systemctl/systemctl-enable.c      |  2 +-

 src/systemctl/systemctl-sysv-compat.c |  2 +-

 src/sysv-generator/sysv-generator.c   |  4 +-

 src/test/test-fileio.c                |  3 +-

 src/test/test-os-util.c               |  7 +--

 12 files changed, 84 insertions(+), 101 deletions(-)

diff --git a/src/basic/env-file.c b/src/basic/env-file.c

index 0353f3f2a0..0e272da083 100644

--- 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;

@@ -100,7 +99,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;

@@ -143,7 +142,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;

@@ -262,7 +261,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;

@@ -300,8 +299,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;

@@ -323,9 +321,6 @@ static int parse_env_file_push(

                         free(*v);

                         *v = value;

-                        if (n_pushed)

-                                (*n_pushed)++;

-

                         return 1;

                 }

         }

@@ -341,16 +336,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(

@@ -371,8 +363,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;

@@ -389,34 +380,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);

@@ -427,49 +412,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;

@@ -498,7 +471,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(

@@ -510,7 +483,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 921a30cef7..ab51955e34 100644

--- a/src/basic/path-lookup.c

+++ b/src/basic/path-lookup.c

@@ -509,7 +509,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) {

@@ -527,7 +527,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);

@@ -717,7 +717,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),

@@ -742,41 +742,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 af85dc7b4f..1f0e5ea271 100644

--- 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 12c49e7fca..22bd0866c5 100644

--- a/src/core/manager.c

+++ b/src/core/manager.c

@@ -1756,11 +1756,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);

@@ -3302,9 +3302,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 68fbbf643a..21f3714eba 100644

--- a/src/shared/condition.c

+++ b/src/shared/condition.c

@@ -787,7 +787,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 a541d32fb7..f1a8b7eb9b 100644

--- a/src/shared/install.c

+++ b/src/shared/install.c

@@ -2615,7 +2615,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 a97aa7be4c..92abd15636 100644

--- a/src/systemctl/systemctl-edit.c

+++ b/src/systemctl/systemctl-edit.c

@@ -38,9 +38,9 @@ int 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)

@@ -511,9 +511,9 @@ int 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 dcbe2c7302..7860f3dc6c 100644

--- a/src/systemctl/systemctl-enable.c

+++ b/src/systemctl/systemctl-enable.c

@@ -142,7 +142,7 @@ int enable_unit(int argc, char *argv[], void *userdata) {

                         char **name;

                         _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 017dba2034..c6e8defd1b 100644

--- 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 e9976540b5..bb74b486be 100644

--- a/src/sysv-generator/sysv-generator.c

+++ b/src/sysv-generator/sysv-generator.c

@@ -894,9 +894,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 4f91d94709..238ae8f586 100644

--- a/src/test/test-fileio.c

+++ b/src/test/test-fileio.c

@@ -110,8 +110,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 d6336c53e9..2cee6470c4 100644

--- 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"));