From 8fa3444f3ca7add9af40ab565e045c2754e5a855 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= 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"));