From 010454b459ef59bab2f9ddd994e4f50491ced371 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 16 Oct 2016 21:20:08 -0400 Subject: shared/install: do not break loop when we enounter a dangling symlink We should ignore that unit, but otherwise continue. --- src/shared/install.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index 60a6d1312d..bcb169e7df 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1767,7 +1767,7 @@ static int install_context_mark_for_removal( r = install_info_traverse(scope, c, paths, i, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, NULL); if (r == -ENOLINK) - return 0; + continue; else if (r < 0) return r; -- cgit v1.2.3-54-g00ecf From 19539807b597274275271c82113e8eb2850bb19f Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 16 Oct 2016 20:56:31 -0400 Subject: shared/install: in install_context_mark_for_removal ignore not found units With the following test case: [Install] WantedBy= default.target Also=foobar-unknown.service disabling would fail with: $ ./systemctl --root=/ disable testing.service Cannot find unit foobar-unknown.service. # this is level debug Failed to disable: No such file or directory. # this is the error After the change we proceed: $ ./systemctl --root=/ disable testing.service Cannot find unit foobar-unknown.service. Removed /etc/systemd/system/default.target.wants/testing.service. This does not affect specifying a missing unit directly: $ ./systemctl --root=/ disable nosuch.service Failed to disable: No such file or directory. --- src/shared/install.c | 33 ++++++++++++++++++++++----------- src/shared/install.h | 4 ++-- 2 files changed, 24 insertions(+), 13 deletions(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index bcb169e7df..9e26284ade 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -934,11 +934,14 @@ static int install_info_may_process( /** * Adds a new UnitFileInstallInfo entry under name in the InstallContext.will_process * hashmap, or retrieves the existing one if already present. + * + * Returns negative on error, 0 if the unit was already known, 1 otherwise. */ static int install_info_add( InstallContext *c, const char *name, const char *path, + bool auxiliary, UnitFileInstallInfo **ret) { UnitFileInstallInfo *i = NULL; @@ -955,6 +958,8 @@ static int install_info_add( i = install_info_find(c, name); if (i) { + i->auxiliary = i->auxiliary && auxiliary; + if (ret) *ret = i; return 0; @@ -968,6 +973,7 @@ static int install_info_add( if (!i) return -ENOMEM; i->type = _UNIT_FILE_TYPE_INVALID; + i->auxiliary = auxiliary; i->name = strdup(name); if (!i->name) { @@ -990,7 +996,7 @@ static int install_info_add( if (ret) *ret = i; - return 0; + return 1; fail: install_info_free(i); @@ -1039,7 +1045,7 @@ static int config_parse_also( void *data, void *userdata) { - UnitFileInstallInfo *i = userdata; + UnitFileInstallInfo *info = userdata, *alsoinfo = NULL; InstallContext *c = data; int r; @@ -1056,11 +1062,11 @@ static int config_parse_also( if (r == 0) break; - r = install_info_add(c, word, NULL, NULL); + r = install_info_add(c, word, NULL, true, &alsoinfo); if (r < 0) return r; - r = strv_push(&i->also, word); + r = strv_push(&info->also, word); if (r < 0) return r; @@ -1432,7 +1438,7 @@ static int install_info_traverse( bn = buffer; } - r = install_info_add(c, bn, NULL, &i); + r = install_info_add(c, bn, NULL, false, &i); if (r < 0) return r; @@ -1471,9 +1477,9 @@ static int install_info_add_auto( pp = prefix_roota(paths->root_dir, name_or_path); - return install_info_add(c, NULL, pp, ret); + return install_info_add(c, NULL, pp, false, ret); } else - return install_info_add(c, name_or_path, NULL, ret); + return install_info_add(c, name_or_path, NULL, false, ret); } static int install_info_discover( @@ -1766,10 +1772,15 @@ static int install_context_mark_for_removal( return r; r = install_info_traverse(scope, c, paths, i, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, NULL); - if (r == -ENOLINK) + if (r == -ENOLINK) { + log_debug_errno(r, "Name %s leads to a dangling symlink, ignoring.", i->name); continue; - else if (r < 0) - return r; + } else if (r == -ENOENT && i->auxiliary) { + /* some unit specified in Also= or similar is missing */ + log_debug_errno(r, "Auxiliary unit %s not found, ignoring.", i->name); + continue; + } else if (r < 0) + return log_debug_errno(r, "Failed to find unit %s: %m", i->name); if (i->type != UNIT_FILE_TYPE_REGULAR) { log_debug("Unit %s has type %s, ignoring.", @@ -2316,7 +2327,7 @@ int unit_file_disable( if (!unit_name_is_valid(*i, UNIT_NAME_ANY)) return -EINVAL; - r = install_info_add(&c, *i, NULL, NULL); + r = install_info_add(&c, *i, NULL, false, NULL); if (r < 0) return r; } diff --git a/src/shared/install.h b/src/shared/install.h index c6aa4f6ef1..b1f220693b 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -119,10 +119,10 @@ struct UnitFileInstallInfo { char **also; char *default_instance; + char *symlink_target; UnitFileType type; - - char *symlink_target; + bool auxiliary; }; static inline bool UNIT_FILE_INSTALL_INFO_HAS_RULES(UnitFileInstallInfo *i) { -- cgit v1.2.3-54-g00ecf From db093eed04cf88aa75c58cb7a69d55d669af0e34 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 16 Oct 2016 21:27:57 -0400 Subject: shared/install: provide more info if install_info_traverse_fails Test case: [Install] WantedBy= default.target Also=foobar-unknown.service Before: $ systemctl --root=/ enable testing2@instance.service Failed to enable: No such file or directory. After $ ./systemctl --root=/ enable testing2@instance.service Failed to enable unit, file foobar-unknown.service: No such file or directory. --- src/shared/install.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index 9e26284ade..32f7edc8cc 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1713,8 +1713,10 @@ static int install_context_apply( return q; r = install_info_traverse(scope, c, paths, i, flags, NULL); - if (r < 0) + if (r < 0) { + unit_file_changes_add(changes, n_changes, r, i->name, NULL); return r; + } /* We can attempt to process a masked unit when a different unit * that we were processing specifies it in Also=. */ -- cgit v1.2.3-54-g00ecf From a6612e658c19717a51366fa2e4925e557d95a427 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 16 Oct 2016 21:40:41 -0400 Subject: shared/install: resolve specifiers in Also= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test case: [Install] WantedBy= default.target Also=getty@%p.service $ ./systemctl --root=/ enable testing@instance.service Created symlink /etc/systemd/system/default.target.wants/testing@instance.service → /etc/systemd/system/testing@.service. Created symlink /etc/systemd/system/getty.target.wants/getty@testing.service → /usr/lib/systemd/system/getty@.service. $ ./systemctl --root=/ disable testing@instance.service Removed /etc/systemd/system/getty.target.wants/getty@testing.service. Removed /etc/systemd/system/default.target.wants/testing@instance.service. Fixes part of #4210. Resolving specifiers in DefaultInstance seems to work too: [Install] WantedBy= default.target DefaultInstance=%u $ systemctl --root=/ enable testing3@instance.service Created symlink /etc/systemd/system/default.target.wants/testing3@instance.service → /etc/systemd/system/testing3@.service. $ systemctl --root=/ enable testing3@.service Created symlink /etc/systemd/system/default.target.wants/testing3@zbyszek.service → /etc/systemd/system/testing3@.service. --- src/shared/install.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index 32f7edc8cc..ff1ecbe7ff 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1054,7 +1054,7 @@ static int config_parse_also( assert(rvalue); for (;;) { - _cleanup_free_ char *word = NULL; + _cleanup_free_ char *word = NULL, *printed = NULL; r = extract_first_word(&rvalue, &word, NULL, 0); if (r < 0) @@ -1062,15 +1062,22 @@ static int config_parse_also( if (r == 0) break; - r = install_info_add(c, word, NULL, true, &alsoinfo); + r = install_full_printf(info, word, &printed); if (r < 0) return r; - r = strv_push(&info->also, word); + if (!unit_name_is_valid(printed, UNIT_NAME_ANY)) + return -EINVAL; + + r = install_info_add(c, printed, NULL, true, &alsoinfo); + if (r < 0) + return r; + + r = strv_push(&info->also, printed); if (r < 0) return r; - word = NULL; + printed = NULL; } return 0; -- cgit v1.2.3-54-g00ecf From 59108fbecb831664cb968ad04acb77c55a756eb1 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 16 Oct 2016 22:57:38 -0400 Subject: shared/install: report invalid unit files slightly better When a unit file is invalid, we'd return an error without any details: $ systemctl --root=/ enable testing@instance.service Failed to enable: Invalid argument. Fix things to at least print the offending file name: $ systemctl enable testing@instance.service Failed to enable unit: File testing@instance.service: Invalid argument $ systemctl --root=/ enable testing@instance.service Failed to enable unit, file testing@instance.service: Invalid argument. A real fix would be to pass back a proper error message from conf-parser. But this would require major surgery, since conf-parser functions now simply print log errors, but we would need to return them over the bus. So let's just print the file name, to indicate where the error is. (Incomplete) fix for #4210. --- src/shared/install.c | 42 +++++++++++++++++++++++++++--------------- src/test/test-install-root.c | 4 +++- 2 files changed, 30 insertions(+), 16 deletions(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index ff1ecbe7ff..f44f80560a 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1205,7 +1205,7 @@ static int unit_file_load( config_item_table_lookup, items, true, true, false, info); if (r < 0) - return r; + return log_debug_errno(r, "Failed to parse %s: %m", info->name); info->type = UNIT_FILE_TYPE_REGULAR; @@ -1495,7 +1495,9 @@ static int install_info_discover( const LookupPaths *paths, const char *name, SearchFlags flags, - UnitFileInstallInfo **ret) { + UnitFileInstallInfo **ret, + UnitFileChange **changes, + unsigned *n_changes) { UnitFileInstallInfo *i; int r; @@ -1505,10 +1507,12 @@ static int install_info_discover( assert(name); r = install_info_add_auto(c, paths, name, &i); - if (r < 0) - return r; + if (r >= 0) + r = install_info_traverse(scope, c, paths, i, flags, ret); - return install_info_traverse(scope, c, paths, i, flags, ret); + if (r < 0) + unit_file_changes_add(changes, n_changes, r, name, NULL); + return r; } static int install_info_symlink_alias( @@ -2225,7 +2229,8 @@ int unit_file_add_dependency( config_path = runtime ? paths.runtime_config : paths.persistent_config; - r = install_info_discover(scope, &c, &paths, target, SEARCH_FOLLOW_CONFIG_SYMLINKS, &target_info); + r = install_info_discover(scope, &c, &paths, target, SEARCH_FOLLOW_CONFIG_SYMLINKS, + &target_info, changes, n_changes); if (r < 0) return r; r = install_info_may_process(target_info, &paths, changes, n_changes); @@ -2237,7 +2242,8 @@ int unit_file_add_dependency( STRV_FOREACH(f, files) { char ***l; - r = install_info_discover(scope, &c, &paths, *f, SEARCH_FOLLOW_CONFIG_SYMLINKS, &i); + r = install_info_discover(scope, &c, &paths, *f, SEARCH_FOLLOW_CONFIG_SYMLINKS, + &i, changes, n_changes); if (r < 0) return r; r = install_info_may_process(i, &paths, changes, n_changes); @@ -2290,7 +2296,8 @@ int unit_file_enable( config_path = runtime ? paths.runtime_config : paths.persistent_config; STRV_FOREACH(f, files) { - r = install_info_discover(scope, &c, &paths, *f, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, &i); + r = install_info_discover(scope, &c, &paths, *f, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, + &i, changes, n_changes); if (r < 0) return r; r = install_info_may_process(i, &paths, changes, n_changes); @@ -2403,7 +2410,7 @@ int unit_file_set_default( if (r < 0) return r; - r = install_info_discover(scope, &c, &paths, name, 0, &i); + r = install_info_discover(scope, &c, &paths, name, 0, &i, changes, n_changes); if (r < 0) return r; r = install_info_may_process(i, &paths, changes, n_changes); @@ -2433,7 +2440,8 @@ int unit_file_get_default( if (r < 0) return r; - r = install_info_discover(scope, &c, &paths, SPECIAL_DEFAULT_TARGET, SEARCH_FOLLOW_CONFIG_SYMLINKS, &i); + r = install_info_discover(scope, &c, &paths, SPECIAL_DEFAULT_TARGET, SEARCH_FOLLOW_CONFIG_SYMLINKS, + &i, NULL, NULL); if (r < 0) return r; r = install_info_may_process(i, &paths, NULL, 0); @@ -2465,7 +2473,8 @@ static int unit_file_lookup_state( if (!unit_name_is_valid(name, UNIT_NAME_ANY)) return -EINVAL; - r = install_info_discover(scope, &c, paths, name, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, &i); + r = install_info_discover(scope, &c, paths, name, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, + &i, NULL, NULL); if (r < 0) return r; @@ -2552,7 +2561,7 @@ int unit_file_exists(UnitFileScope scope, const LookupPaths *paths, const char * if (!unit_name_is_valid(name, UNIT_NAME_ANY)) return -EINVAL; - r = install_info_discover(scope, &c, paths, name, 0, NULL); + r = install_info_discover(scope, &c, paths, name, 0, NULL, NULL, NULL); if (r == -ENOENT) return 0; if (r < 0) @@ -2770,7 +2779,8 @@ static int preset_prepare_one( if (install_info_find(plus, name) || install_info_find(minus, name)) return 0; - r = install_info_discover(scope, &tmp, paths, name, SEARCH_FOLLOW_CONFIG_SYMLINKS, &i); + r = install_info_discover(scope, &tmp, paths, name, SEARCH_FOLLOW_CONFIG_SYMLINKS, + &i, changes, n_changes); if (r < 0) return r; if (!streq(name, i->name)) { @@ -2783,7 +2793,8 @@ static int preset_prepare_one( return r; if (r > 0) { - r = install_info_discover(scope, plus, paths, name, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, &i); + r = install_info_discover(scope, plus, paths, name, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, + &i, changes, n_changes); if (r < 0) return r; @@ -2791,7 +2802,8 @@ static int preset_prepare_one( if (r < 0) return r; } else - r = install_info_discover(scope, minus, paths, name, SEARCH_FOLLOW_CONFIG_SYMLINKS, &i); + r = install_info_discover(scope, minus, paths, name, SEARCH_FOLLOW_CONFIG_SYMLINKS, + &i, changes, n_changes); return r; } diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c index db1c928660..1686054d2a 100644 --- a/src/test/test-install-root.c +++ b/src/test/test-install-root.c @@ -326,7 +326,9 @@ static void test_default(const char *root) { assert_se(unit_file_get_default(UNIT_FILE_SYSTEM, root, &def) == -ENOENT); assert_se(unit_file_set_default(UNIT_FILE_SYSTEM, root, "idontexist.target", false, &changes, &n_changes) == -ENOENT); - assert_se(n_changes == 0); + assert_se(n_changes == 1); + assert_se(changes[0].type == -ENOENT); + assert_se(streq_ptr(changes[0].path, "idontexist.target")); unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; -- cgit v1.2.3-54-g00ecf