From 3ec530a1890925efe347f739917dd4078c1b1942 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 21 Sep 2017 16:05:52 +0200 Subject: [PATCH 1/5] timedatectl: be more explicit what "ntp synchronized" means The documentation explained that the message doesn't really mean what it says, but I think it's better to just make the message more straightforward. Fixes #6554. --- man/timedatectl.xml | 25 +++++++++++-------------- src/timedate/timedatectl.c | 20 ++++++++++---------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/man/timedatectl.xml b/man/timedatectl.xml index d8a83c8add..0dacd05958 100644 --- a/man/timedatectl.xml +++ b/man/timedatectl.xml @@ -104,13 +104,10 @@ status Show current settings of the system clock and - RTC, including whether network time synchronization is - on. Note that whether network time synchronization is on - simply reflects whether the - systemd-timesyncd.service unit is - enabled. Even if this command shows the status as off, a - different service might still synchronize the clock with the - network. + RTC, including whether network time synchronization through + systemd-timesyncd.service is active. + Even if is off, a different service might still synchronize the + clock with the network. @@ -206,13 +203,13 @@ Examples Show current settings: $ timedatectl - Local time: Di 2015-04-07 16:26:56 CEST - Universal time: Di 2015-04-07 14:26:56 UTC - RTC time: Di 2015-04-07 14:26:56 - Time zone: Europe/Berlin (CEST, +0200) - Network time on: yes -NTP synchronized: yes - RTC in local TZ: no + Local time: Thu 2017-09-21 16:08:56 CEST + Universal time: Thu 2017-09-21 14:08:56 UTC + RTC time: Thu 2017-09-21 14:08:56 + Time zone: Europe/Warsaw (CEST, +0200) + System clock synchronized: yes +systemd-timesyncd.service active: yes + RTC in local TZ: no Enable network time synchronization: diff --git a/src/timedate/timedatectl.c b/src/timedate/timedatectl.c index 281b1534a3..a30e783c09 100644 --- a/src/timedate/timedatectl.c +++ b/src/timedate/timedatectl.c @@ -103,13 +103,13 @@ static void print_status_info(const StatusInfo *i) { if (have_time) { xstrftime(a, "%a %Y-%m-%d %H:%M:%S %Z", localtime_r(&sec, &tm)); - printf(" Local time: %.*s\n", (int) sizeof(a), a); + printf(" Local time: %.*s\n", (int) sizeof(a), a); xstrftime(a, "%a %Y-%m-%d %H:%M:%S UTC", gmtime_r(&sec, &tm)); - printf(" Universal time: %.*s\n", (int) sizeof(a), a); + printf(" Universal time: %.*s\n", (int) sizeof(a), a); } else { - printf(" Local time: %s\n", "n/a"); - printf(" Universal time: %s\n", "n/a"); + printf(" Local time: %s\n", "n/a"); + printf(" Universal time: %s\n", "n/a"); } if (i->rtc_time > 0) { @@ -117,9 +117,9 @@ static void print_status_info(const StatusInfo *i) { rtc_sec = (time_t) (i->rtc_time / USEC_PER_SEC); xstrftime(a, "%a %Y-%m-%d %H:%M:%S", gmtime_r(&rtc_sec, &tm)); - printf(" RTC time: %.*s\n", (int) sizeof(a), a); + printf(" RTC time: %.*s\n", (int) sizeof(a), a); } else - printf(" RTC time: %s\n", "n/a"); + printf(" RTC time: %s\n", "n/a"); if (have_time) xstrftime(a, "%Z, %z", localtime_r(&sec, &tm)); @@ -134,10 +134,10 @@ static void print_status_info(const StatusInfo *i) { else tzset(); - printf(" Time zone: %s (%.*s)\n" - " Network time on: %s\n" - "NTP synchronized: %s\n" - " RTC in local TZ: %s\n", + printf(" Time zone: %s (%.*s)\n" + " System clock synchronized: %s\n" + "systemd-timesyncd.service active: %s\n" + " RTC in local TZ: %s\n", strna(i->timezone), (int) sizeof(a), have_time ? a : "n/a", i->ntp_capable ? yes_no(i->ntp_enabled) : "n/a", yes_no(i->ntp_synced), From 7a7ec2bf71b146aaa217732d2ad029e730ffb3cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 21 Sep 2017 18:36:45 +0200 Subject: [PATCH 2/5] install: move and rename to lowercase two functions No reason to make them look like macros. --- src/shared/install.c | 18 ++++++++++++++++-- src/shared/install.h | 14 -------------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 9e4e0eaf6f..d73727e403 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -83,6 +83,20 @@ typedef struct { size_t n_rules; } Presets; +static inline bool unit_file_install_info_has_rules(UnitFileInstallInfo *i) { + assert(i); + + return !strv_isempty(i->aliases) || + !strv_isempty(i->wanted_by) || + !strv_isempty(i->required_by); +} + +static inline bool unit_file_install_info_has_also(UnitFileInstallInfo *i) { + assert(i); + + return !strv_isempty(i->also); +} + static inline void presets_freep(Presets *p) { size_t i; @@ -2593,9 +2607,9 @@ static int unit_file_lookup_state( if (r < 0) return r; if (r == 0) { - if (UNIT_FILE_INSTALL_INFO_HAS_RULES(i)) + if (unit_file_install_info_has_rules(i)) state = UNIT_FILE_DISABLED; - else if (UNIT_FILE_INSTALL_INFO_HAS_ALSO(i)) + else if (unit_file_install_info_has_also(i)) state = UNIT_FILE_INDIRECT; else state = UNIT_FILE_STATIC; diff --git a/src/shared/install.h b/src/shared/install.h index 7a5859e729..c1fcbe96ed 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -132,20 +132,6 @@ struct UnitFileInstallInfo { bool auxiliary; }; -static inline bool UNIT_FILE_INSTALL_INFO_HAS_RULES(UnitFileInstallInfo *i) { - assert(i); - - return !strv_isempty(i->aliases) || - !strv_isempty(i->wanted_by) || - !strv_isempty(i->required_by); -} - -static inline bool UNIT_FILE_INSTALL_INFO_HAS_ALSO(UnitFileInstallInfo *i) { - assert(i); - - return !strv_isempty(i->also); -} - bool unit_type_may_alias(UnitType type) _const_; bool unit_type_may_template(UnitType type) _const_; From 5cd8ae31527384dcdbd1e9d2c7e7cd3a803222f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 21 Sep 2017 18:53:45 +0200 Subject: [PATCH 3/5] install: only consider names in Alias= as "enabling" When a unit has a symlink that makes an alias in the filesystem, but that name is not specified in [Install], it is confusing is the unit is shown as "enabled". Look only for names specified in Alias=. Fixes #6338. v2: - Fix indentation. - Fix checking for normal enablement, when the symlink name is the same as the unit name. This case wasn't handled properly in v1. v3: - Rework the patch to also handle templates properly: A template templ@.service with DefaultInstance=foo will be considered enabled only when templ@foo.service symlink is found. Symlinks with other instance names do not count, which matches the logic for aliases to normal units. Tests are updated. --- man/systemctl.xml | 2 +- src/shared/install.c | 77 ++++++++++++++++++++++++++---------- src/test/test-install-root.c | 14 +++++-- 3 files changed, 69 insertions(+), 24 deletions(-) diff --git a/man/systemctl.xml b/man/systemctl.xml index 0778c38ca4..cf9d85c98c 100644 --- a/man/systemctl.xml +++ b/man/systemctl.xml @@ -1245,7 +1245,7 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err enabled - Enabled via .wants/, .requires/ or alias symlinks (permanently in /etc/systemd/system/, or transiently in /run/systemd/system/). + Enabled via .wants/, .requires/ or Alias= symlinks (permanently in /etc/systemd/system/, or transiently in /run/systemd/system/). 0 diff --git a/src/shared/install.c b/src/shared/install.c index d73727e403..f668445282 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -699,9 +699,34 @@ static int remove_marked_symlinks( return r; } +static bool is_symlink_with_known_name(const UnitFileInstallInfo *i, const char *name) { + int r; + + if (streq(name, i->name)) + return true; + + if (strv_contains(i->aliases, name)) + return true; + + /* Look for template symlink matching DefaultInstance */ + if (i->default_instance && unit_name_is_valid(i->name, UNIT_NAME_TEMPLATE)) { + _cleanup_free_ char *s = NULL; + + r = unit_name_replace_instance(i->name, i->default_instance, &s); + if (r < 0) { + if (r != -EINVAL) + return r; + + } else if (streq(name, s)) + return true; + } + + return false; +} + static int find_symlinks_fd( const char *root_dir, - const char *name, + UnitFileInstallInfo *i, int fd, const char *path, const char *config_path, @@ -711,7 +736,7 @@ static int find_symlinks_fd( struct dirent *de; int r = 0; - assert(name); + assert(i); assert(fd >= 0); assert(path); assert(config_path); @@ -748,7 +773,8 @@ static int find_symlinks_fd( } /* This will close nfd, regardless whether it succeeds or not */ - q = find_symlinks_fd(root_dir, name, nfd, p, config_path, same_name_link); + q = find_symlinks_fd(root_dir, i, nfd, + p, config_path, same_name_link); if (q > 0) return 1; if (r == 0) @@ -788,24 +814,24 @@ static int find_symlinks_fd( /* Check if the symlink itself matches what we * are looking for */ - if (path_is_absolute(name)) - found_path = path_equal(p, name); + if (path_is_absolute(i->name)) + found_path = path_equal(p, i->name); else - found_path = streq(de->d_name, name); + found_path = streq(de->d_name, i->name); /* Check if what the symlink points to * matches what we are looking for */ - if (path_is_absolute(name)) - found_dest = path_equal(dest, name); + if (path_is_absolute(i->name)) + found_dest = path_equal(dest, i->name); else - found_dest = streq(basename(dest), name); + found_dest = streq(basename(dest), i->name); if (found_path && found_dest) { _cleanup_free_ char *t = NULL; /* Filter out same name links in the main * config path */ - t = path_make_absolute(name, config_path); + t = path_make_absolute(i->name, config_path); if (!t) return -ENOMEM; @@ -814,8 +840,15 @@ static int find_symlinks_fd( if (b) *same_name_link = true; - else if (found_path || found_dest) - return 1; + else if (found_path || found_dest) { + /* Check if symlink name is in the set of names used by [Install] */ + q = is_symlink_with_known_name(i, de->d_name); + log_info("is_symlink_with_known_name(%s, %s) → %d", i->name, de->d_name, q); + if (q < 0) + return q; + if (q > 0) + return 1; + } } } @@ -824,13 +857,13 @@ static int find_symlinks_fd( static int find_symlinks( const char *root_dir, - const char *name, + UnitFileInstallInfo *i, const char *config_path, bool *same_name_link) { int fd; - assert(name); + assert(i); assert(config_path); assert(same_name_link); @@ -842,12 +875,13 @@ static int find_symlinks( } /* This takes possession of fd and closes it */ - return find_symlinks_fd(root_dir, name, fd, config_path, config_path, same_name_link); + return find_symlinks_fd(root_dir, i, fd, + config_path, config_path, same_name_link); } static int find_symlinks_in_scope( const LookupPaths *paths, - const char *name, + UnitFileInstallInfo *i, UnitFileState *state) { bool same_name_link_runtime = false, same_name_link_config = false; @@ -856,12 +890,12 @@ static int find_symlinks_in_scope( int r; assert(paths); - assert(name); + assert(i); STRV_FOREACH(p, paths->search_path) { bool same_name_link = false; - r = find_symlinks(paths->root_dir, name, *p, &same_name_link); + r = find_symlinks(paths->root_dir, i, *p, &same_name_link); if (r < 0) return r; if (r > 0) { @@ -910,7 +944,7 @@ static int find_symlinks_in_scope( * outside of runtime and configuration directory, then we consider it statically enabled. Note we do that only * for instance, not for regular names, as those are merely aliases, while instances explicitly instantiate * something, and hence are a much stronger concept. */ - if (enabled_at_all && unit_name_is_valid(name, UNIT_NAME_INSTANCE)) { + if (enabled_at_all && unit_name_is_valid(i->name, UNIT_NAME_INSTANCE)) { *state = UNIT_FILE_STATIC; return 1; } @@ -2603,7 +2637,10 @@ static int unit_file_lookup_state( break; } - r = find_symlinks_in_scope(paths, i->name, &state); + /* Check if any of the Alias= symlinks have been created. + * We ignore other aliases, and only check those that would + * be created by systemctl enable for this unit. */ + r = find_symlinks_in_scope(paths, i, &state); if (r < 0) return r; if (r == 0) { diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c index 575401cb91..bb61d7fb19 100644 --- a/src/test/test-install-root.c +++ b/src/test/test-install-root.c @@ -381,6 +381,8 @@ static void test_template_enable(const char *root) { UnitFileState state; const char *p; + log_info("== %s ==", __func__); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@.service", &state) == -ENOENT); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@def.service", &state) == -ENOENT); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@foo.service", &state) == -ENOENT); @@ -402,6 +404,8 @@ static void test_template_enable(const char *root) { assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@def.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@foo.service", &state) >= 0 && state == UNIT_FILE_DISABLED); + log_info("== %s with template@.service enabled ==", __func__); + assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("template@.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); assert_se(changes[0].type == UNIT_FILE_SYMLINK); @@ -432,6 +436,8 @@ static void test_template_enable(const char *root) { assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@def.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@foo.service", &state) >= 0 && state == UNIT_FILE_DISABLED); + log_info("== %s with template@foo.service enabled ==", __func__); + assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("template@foo.service"), &changes, &n_changes) >= 0); assert_se(changes[0].type == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/template@.service")); @@ -440,7 +446,7 @@ static void test_template_enable(const char *root) { unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; - assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@.service", &state) >= 0 && state == UNIT_FILE_ENABLED); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@def.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@foo.service", &state) >= 0 && state == UNIT_FILE_ENABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@foo.service", &state) >= 0 && state == UNIT_FILE_ENABLED); @@ -463,6 +469,8 @@ static void test_template_enable(const char *root) { assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@foo.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@quux.service", &state) >= 0 && state == UNIT_FILE_DISABLED); + log_info("== %s with template-symlink@quux.service enabled ==", __func__); + assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("template-symlink@quux.service"), &changes, &n_changes) >= 0); assert_se(changes[0].type == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/template@.service")); @@ -471,11 +479,11 @@ static void test_template_enable(const char *root) { unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; - assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@.service", &state) >= 0 && state == UNIT_FILE_ENABLED); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@def.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@foo.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@quux.service", &state) >= 0 && state == UNIT_FILE_ENABLED); - assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@.service", &state) >= 0 && state == UNIT_FILE_ENABLED); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@def.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@foo.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@quux.service", &state) >= 0 && state == UNIT_FILE_ENABLED); From d9b4b48f3fee12293cc911929d1d92a01a5c69c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 21 Sep 2017 19:03:17 +0200 Subject: [PATCH 4/5] install: consider non-Alias=/non-DefaultInstance= symlinks as "indirect" enablement I think this matches the spirit of "indirect" well: the unit *might* be active, even though it is not "installed" in the sense of symlinks created based on the [Install] section. The changes to test-install-root touch the same lines as in the previous commit; the change in each case is from assert_se(unit_file_get_state(...) >= 0 && state == UNIT_FILE_ENABLED) to assert_se(unit_file_get_state(...) >= 0 && state == UNIT_FILE_DISABLED) to assert_se(unit_file_get_state(...) >= 0 && state == UNIT_FILE_INDIRECT) in the last two commits. --- man/systemctl.xml | 2 +- src/shared/install.c | 26 +++++++++++++++++++++----- src/test/test-install-root.c | 6 +++--- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/man/systemctl.xml b/man/systemctl.xml index cf9d85c98c..f514ab64dd 100644 --- a/man/systemctl.xml +++ b/man/systemctl.xml @@ -1274,7 +1274,7 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err indirect - The unit file itself is not enabled, but it has a non-empty Also= setting in the [Install] unit file section, listing other unit files that might be enabled. + The unit file itself is not enabled, but it has a non-empty Also= setting in the [Install] unit file section, listing other unit files that might be enabled, or it has an alias under a different name through a symlink that is not specified in Also=. For template unit file, an instance different than the one specified in DefaultInstance= is enabled. 0 diff --git a/src/shared/install.c b/src/shared/install.c index f668445282..4141e0402f 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -727,6 +727,7 @@ static bool is_symlink_with_known_name(const UnitFileInstallInfo *i, const char static int find_symlinks_fd( const char *root_dir, UnitFileInstallInfo *i, + bool match_aliases, int fd, const char *path, const char *config_path, @@ -773,7 +774,7 @@ static int find_symlinks_fd( } /* This will close nfd, regardless whether it succeeds or not */ - q = find_symlinks_fd(root_dir, i, nfd, + q = find_symlinks_fd(root_dir, i, match_aliases, nfd, p, config_path, same_name_link); if (q > 0) return 1; @@ -841,6 +842,9 @@ static int find_symlinks_fd( if (b) *same_name_link = true; else if (found_path || found_dest) { + if (!match_aliases) + return 1; + /* Check if symlink name is in the set of names used by [Install] */ q = is_symlink_with_known_name(i, de->d_name); log_info("is_symlink_with_known_name(%s, %s) → %d", i->name, de->d_name, q); @@ -858,6 +862,7 @@ static int find_symlinks_fd( static int find_symlinks( const char *root_dir, UnitFileInstallInfo *i, + bool match_name, const char *config_path, bool *same_name_link) { @@ -875,13 +880,14 @@ static int find_symlinks( } /* This takes possession of fd and closes it */ - return find_symlinks_fd(root_dir, i, fd, + return find_symlinks_fd(root_dir, i, match_name, fd, config_path, config_path, same_name_link); } static int find_symlinks_in_scope( const LookupPaths *paths, UnitFileInstallInfo *i, + bool match_name, UnitFileState *state) { bool same_name_link_runtime = false, same_name_link_config = false; @@ -895,7 +901,7 @@ static int find_symlinks_in_scope( STRV_FOREACH(p, paths->search_path) { bool same_name_link = false; - r = find_symlinks(paths->root_dir, i, *p, &same_name_link); + r = find_symlinks(paths->root_dir, i, match_name, *p, &same_name_link); if (r < 0) return r; if (r > 0) { @@ -2640,10 +2646,20 @@ static int unit_file_lookup_state( /* Check if any of the Alias= symlinks have been created. * We ignore other aliases, and only check those that would * be created by systemctl enable for this unit. */ - r = find_symlinks_in_scope(paths, i, &state); + r = find_symlinks_in_scope(paths, i, true, &state); if (r < 0) return r; - if (r == 0) { + if (r > 0) + break; + + /* Check if the file is known under other names. If it is, + * it might be in use. Report that as UNIT_FILE_INDIRECT. */ + r = find_symlinks_in_scope(paths, i, false, &state); + if (r < 0) + return r; + if (r > 0) + state = UNIT_FILE_INDIRECT; + else { if (unit_file_install_info_has_rules(i)) state = UNIT_FILE_DISABLED; else if (unit_file_install_info_has_also(i)) diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c index bb61d7fb19..e2754a0773 100644 --- a/src/test/test-install-root.c +++ b/src/test/test-install-root.c @@ -446,7 +446,7 @@ static void test_template_enable(const char *root) { unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; - assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@.service", &state) >= 0 && state == UNIT_FILE_DISABLED); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@.service", &state) >= 0 && state == UNIT_FILE_INDIRECT); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@def.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@foo.service", &state) >= 0 && state == UNIT_FILE_ENABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@foo.service", &state) >= 0 && state == UNIT_FILE_ENABLED); @@ -479,11 +479,11 @@ static void test_template_enable(const char *root) { unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; - assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@.service", &state) >= 0 && state == UNIT_FILE_DISABLED); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@.service", &state) >= 0 && state == UNIT_FILE_INDIRECT); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@def.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@foo.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template@quux.service", &state) >= 0 && state == UNIT_FILE_ENABLED); - assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@.service", &state) >= 0 && state == UNIT_FILE_DISABLED); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@.service", &state) >= 0 && state == UNIT_FILE_INDIRECT); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@def.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@foo.service", &state) >= 0 && state == UNIT_FILE_DISABLED); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "template-symlink@quux.service", &state) >= 0 && state == UNIT_FILE_ENABLED); From d2561cfdf772c621963e62b52cfbf57d6f75178d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 22 Sep 2017 14:46:09 +0200 Subject: [PATCH 5/5] install: consider globally enabled units as "enabled" for the user We would not consider symlinks in /etc/systemd/user/*.{wants,requires}/ towards the user unit being "enabled", because the symlinks were not located in "config" paths. But this is confusing to users, since those units are clearly enabled and will be started. So let's muddle the definition of enablement a bit to include the paths only accessible to root when looking for enabled user units. Fixes #4432. --- src/shared/install.c | 11 ++++++++-- src/shared/path-lookup.c | 46 +++++++++++++++++++++++++--------------- src/shared/path-lookup.h | 2 ++ 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 4141e0402f..e14a869321 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -885,6 +885,7 @@ static int find_symlinks( } static int find_symlinks_in_scope( + UnitFileScope scope, const LookupPaths *paths, UnitFileInstallInfo *i, bool match_name, @@ -916,6 +917,12 @@ static int find_symlinks_in_scope( return 1; } + /* look for globally enablement of user units */ + if (scope == UNIT_FILE_USER && path_is_user_config_dir(*p)) { + *state = UNIT_FILE_ENABLED; + return 1; + } + r = path_is_runtime(paths, *p, false); if (r < 0) return r; @@ -2646,7 +2653,7 @@ static int unit_file_lookup_state( /* Check if any of the Alias= symlinks have been created. * We ignore other aliases, and only check those that would * be created by systemctl enable for this unit. */ - r = find_symlinks_in_scope(paths, i, true, &state); + r = find_symlinks_in_scope(scope, paths, i, true, &state); if (r < 0) return r; if (r > 0) @@ -2654,7 +2661,7 @@ static int unit_file_lookup_state( /* Check if the file is known under other names. If it is, * it might be in use. Report that as UNIT_FILE_INDIRECT. */ - r = find_symlinks_in_scope(paths, i, false, &state); + r = find_symlinks_in_scope(scope, paths, i, false, &state); if (r < 0) return r; if (r > 0) diff --git a/src/shared/path-lookup.c b/src/shared/path-lookup.c index a289511be5..bf10acda94 100644 --- a/src/shared/path-lookup.c +++ b/src/shared/path-lookup.c @@ -115,6 +115,21 @@ static int user_data_dir(char **ret, const char *suffix) { return 1; } +static const char* const user_data_unit_paths[] = { + "/usr/local/lib/systemd/user", + "/usr/local/share/systemd/user", + USER_DATA_UNIT_PATH, + "/usr/lib/systemd/user", + "/usr/share/systemd/user", + NULL +}; + +static const char* const user_config_unit_paths[] = { + USER_CONFIG_UNIT_PATH, + "/etc/systemd/user", + NULL +}; + static char** user_dirs( const char *persistent_config, const char *runtime_config, @@ -125,21 +140,6 @@ static char** user_dirs( const char *persistent_control, const char *runtime_control) { - const char * const config_unit_paths[] = { - USER_CONFIG_UNIT_PATH, - "/etc/systemd/user", - NULL - }; - - const char * const data_unit_paths[] = { - "/usr/local/lib/systemd/user", - "/usr/local/share/systemd/user", - USER_DATA_UNIT_PATH, - "/usr/lib/systemd/user", - "/usr/share/systemd/user", - NULL - }; - _cleanup_strv_free_ char **config_dirs = NULL, **data_dirs = NULL; _cleanup_free_ char *data_home = NULL; _cleanup_strv_free_ char **res = NULL; @@ -196,7 +196,7 @@ static char** user_dirs( if (strv_extend(&res, persistent_config) < 0) return NULL; - if (strv_extend_strv(&res, (char**) config_unit_paths, false) < 0) + if (strv_extend_strv(&res, (char**) user_config_unit_paths, false) < 0) return NULL; if (strv_extend(&res, runtime_config) < 0) @@ -211,7 +211,7 @@ static char** user_dirs( if (strv_extend_strv_concat(&res, data_dirs, "/systemd/user") < 0) return NULL; - if (strv_extend_strv(&res, (char**) data_unit_paths, false) < 0) + if (strv_extend_strv(&res, (char**) user_data_unit_paths, false) < 0) return NULL; if (strv_extend(&res, generator_late) < 0) @@ -226,6 +226,18 @@ static char** user_dirs( return tmp; } +bool path_is_user_data_dir(const char *path) { + assert(path); + + return strv_contains((char**) user_data_unit_paths, path); +} + +bool path_is_user_config_dir(const char *path) { + assert(path); + + return strv_contains((char**) user_config_unit_paths, path); +} + static int acquire_generator_dirs( UnitFileScope scope, const char *tempdir, diff --git a/src/shared/path-lookup.h b/src/shared/path-lookup.h index 45fba64d2a..fc8b8ed8c7 100644 --- a/src/shared/path-lookup.h +++ b/src/shared/path-lookup.h @@ -67,6 +67,8 @@ struct LookupPaths { }; int lookup_paths_init(LookupPaths *p, UnitFileScope scope, LookupPathsFlags flags, const char *root_dir); +bool path_is_user_data_dir(const char *path); +bool path_is_user_config_dir(const char *path); int lookup_paths_reduce(LookupPaths *p);