From 303ee601513d64c23fcff0925894051c3baf0c50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 12 Dec 2018 16:45:33 +0100 Subject: [PATCH 1/5] Mark *data and *userdata params to specifier_printf() as const It would be very wrong if any of the specfier printf calls modified any of the objects or data being printed. Let's mark all arguments as const (primarily to make it easier for the reader to see where modifications cannot occur). --- src/core/cgroup.c | 2 +- src/core/cgroup.h | 2 +- src/core/unit-printf.c | 46 ++++++++++++++++++------------------ src/core/unit.c | 2 +- src/core/unit.h | 2 +- src/resolve/resolved-dnssd.c | 2 +- src/shared/install-printf.c | 13 +++++----- src/shared/install-printf.h | 2 +- src/shared/specifier.c | 28 +++++++++++----------- src/shared/specifier.h | 32 ++++++++++++------------- src/tmpfiles/tmpfiles.c | 8 +++---- 11 files changed, 69 insertions(+), 70 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 5590047976..ae7fc295a0 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1491,7 +1491,7 @@ static const char *migrate_callback(CGroupMask mask, void *userdata) { return unit_get_realized_cgroup_path(userdata, mask); } -char *unit_default_cgroup_path(Unit *u) { +char *unit_default_cgroup_path(const Unit *u) { _cleanup_free_ char *escaped = NULL, *slice = NULL; int r; diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 6d094e9ecd..266daa20a5 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -164,7 +164,7 @@ void unit_invalidate_cgroup_members_masks(Unit *u); void unit_add_to_cgroup_realize_queue(Unit *u); const char *unit_get_realized_cgroup_path(Unit *u, CGroupMask mask); -char *unit_default_cgroup_path(Unit *u); +char *unit_default_cgroup_path(const Unit *u); int unit_set_cgroup_path(Unit *u, const char *path); int unit_pick_cgroup_path(Unit *u); diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c index f57c099c39..a8e84ebe80 100644 --- a/src/core/unit-printf.c +++ b/src/core/unit-printf.c @@ -12,25 +12,25 @@ #include "unit.h" #include "user-util.h" -static int specifier_prefix_and_instance(char specifier, void *data, void *userdata, char **ret) { - Unit *u = userdata; +static int specifier_prefix_and_instance(char specifier, const void *data, const void *userdata, char **ret) { + const Unit *u = userdata; assert(u); return unit_name_to_prefix_and_instance(u->id, ret); } -static int specifier_prefix(char specifier, void *data, void *userdata, char **ret) { - Unit *u = userdata; +static int specifier_prefix(char specifier, const void *data, const void *userdata, char **ret) { + const Unit *u = userdata; assert(u); return unit_name_to_prefix(u->id, ret); } -static int specifier_prefix_unescaped(char specifier, void *data, void *userdata, char **ret) { +static int specifier_prefix_unescaped(char specifier, const void *data, const void *userdata, char **ret) { _cleanup_free_ char *p = NULL; - Unit *u = userdata; + const Unit *u = userdata; int r; assert(u); @@ -42,16 +42,16 @@ static int specifier_prefix_unescaped(char specifier, void *data, void *userdata return unit_name_unescape(p, ret); } -static int specifier_instance_unescaped(char specifier, void *data, void *userdata, char **ret) { - Unit *u = userdata; +static int specifier_instance_unescaped(char specifier, const void *data, const void *userdata, char **ret) { + const Unit *u = userdata; assert(u); return unit_name_unescape(strempty(u->instance), ret); } -static int specifier_last_component(char specifier, void *data, void *userdata, char **ret) { - Unit *u = userdata; +static int specifier_last_component(char specifier, const void *data, const void *userdata, char **ret) { + const Unit *u = userdata; _cleanup_free_ char *prefix = NULL; char *dash; int r; @@ -70,7 +70,7 @@ static int specifier_last_component(char specifier, void *data, void *userdata, return 0; } -static int specifier_last_component_unescaped(char specifier, void *data, void *userdata, char **ret) { +static int specifier_last_component_unescaped(char specifier, const void *data, const void *userdata, char **ret) { _cleanup_free_ char *p = NULL; int r; @@ -81,8 +81,8 @@ static int specifier_last_component_unescaped(char specifier, void *data, void * return unit_name_unescape(p, ret); } -static int specifier_filename(char specifier, void *data, void *userdata, char **ret) { - Unit *u = userdata; +static int specifier_filename(char specifier, const void *data, const void *userdata, char **ret) { + const Unit *u = userdata; assert(u); @@ -92,12 +92,12 @@ static int specifier_filename(char specifier, void *data, void *userdata, char * return unit_name_to_path(u->id, ret); } -static void bad_specifier(Unit *u, char specifier) { +static void bad_specifier(const Unit *u, char specifier) { log_unit_warning(u, "Specifier '%%%c' used in unit configuration, which is deprecated. Please update your unit file, as it does not work as intended.", specifier); } -static int specifier_cgroup(char specifier, void *data, void *userdata, char **ret) { - Unit *u = userdata; +static int specifier_cgroup(char specifier, const void *data, const void *userdata, char **ret) { + const Unit *u = userdata; char *n; assert(u); @@ -115,8 +115,8 @@ static int specifier_cgroup(char specifier, void *data, void *userdata, char **r return 0; } -static int specifier_cgroup_root(char specifier, void *data, void *userdata, char **ret) { - Unit *u = userdata; +static int specifier_cgroup_root(char specifier, const void *data, const void *userdata, char **ret) { + const Unit *u = userdata; char *n; assert(u); @@ -131,8 +131,8 @@ static int specifier_cgroup_root(char specifier, void *data, void *userdata, cha return 0; } -static int specifier_cgroup_slice(char specifier, void *data, void *userdata, char **ret) { - Unit *u = userdata; +static int specifier_cgroup_slice(char specifier, const void *data, const void *userdata, char **ret) { + const Unit *u = userdata; char *n; assert(u); @@ -140,7 +140,7 @@ static int specifier_cgroup_slice(char specifier, void *data, void *userdata, ch bad_specifier(u, specifier); if (UNIT_ISSET(u->slice)) { - Unit *slice; + const Unit *slice; slice = UNIT_DEREF(u->slice); @@ -157,8 +157,8 @@ static int specifier_cgroup_slice(char specifier, void *data, void *userdata, ch return 0; } -static int specifier_special_directory(char specifier, void *data, void *userdata, char **ret) { - Unit *u = userdata; +static int specifier_special_directory(char specifier, const void *data, const void *userdata, char **ret) { + const Unit *u = userdata; char *n = NULL; assert(u); diff --git a/src/core/unit.c b/src/core/unit.c index 171567022f..bc72b5d673 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -132,7 +132,7 @@ int unit_new_for_name(Manager *m, size_t size, const char *name, Unit **ret) { return r; } -bool unit_has_name(Unit *u, const char *name) { +bool unit_has_name(const Unit *u, const char *name) { assert(u); assert(name); diff --git a/src/core/unit.h b/src/core/unit.h index 312742f11b..6fd39eaca3 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -648,7 +648,7 @@ int unit_set_default_slice(Unit *u); const char *unit_description(Unit *u) _pure_; -bool unit_has_name(Unit *u, const char *name); +bool unit_has_name(const Unit *u, const char *name); UnitActiveState unit_active_state(Unit *u); diff --git a/src/resolve/resolved-dnssd.c b/src/resolve/resolved-dnssd.c index 2c28ec227a..b8e6a7abe1 100644 --- a/src/resolve/resolved-dnssd.c +++ b/src/resolve/resolved-dnssd.c @@ -141,7 +141,7 @@ static int dnssd_service_load(Manager *manager, const char *filename) { return 0; } -static int specifier_dnssd_host_name(char specifier, void *data, void *userdata, char **ret) { +static int specifier_dnssd_host_name(char specifier, const void *data, const void *userdata, char **ret) { DnssdService *s = (DnssdService *) userdata; char *n; diff --git a/src/shared/install-printf.c b/src/shared/install-printf.c index b87d6b9d55..d2143beda3 100644 --- a/src/shared/install-printf.c +++ b/src/shared/install-printf.c @@ -14,7 +14,7 @@ #include "unit-name.h" #include "user-util.h" -static int specifier_prefix_and_instance(char specifier, void *data, void *userdata, char **ret) { +static int specifier_prefix_and_instance(char specifier, const void *data, const void *userdata, char **ret) { const UnitFileInstallInfo *i = userdata; _cleanup_free_ char *prefix = NULL; int r; @@ -38,7 +38,7 @@ static int specifier_prefix_and_instance(char specifier, void *data, void *userd return 0; } -static int specifier_name(char specifier, void *data, void *userdata, char **ret) { +static int specifier_name(char specifier, const void *data, const void *userdata, char **ret) { const UnitFileInstallInfo *i = userdata; char *ans; @@ -54,7 +54,7 @@ static int specifier_name(char specifier, void *data, void *userdata, char **ret return 0; } -static int specifier_prefix(char specifier, void *data, void *userdata, char **ret) { +static int specifier_prefix(char specifier, const void *data, const void *userdata, char **ret) { const UnitFileInstallInfo *i = userdata; assert(i); @@ -62,7 +62,7 @@ static int specifier_prefix(char specifier, void *data, void *userdata, char **r return unit_name_to_prefix(i->name, ret); } -static int specifier_instance(char specifier, void *data, void *userdata, char **ret) { +static int specifier_instance(char specifier, const void *data, const void *userdata, char **ret) { const UnitFileInstallInfo *i = userdata; char *instance; int r; @@ -83,7 +83,7 @@ static int specifier_instance(char specifier, void *data, void *userdata, char * return 0; } -static int specifier_last_component(char specifier, void *data, void *userdata, char **ret) { +static int specifier_last_component(char specifier, const void *data, const void *userdata, char **ret) { _cleanup_free_ char *prefix = NULL; char *dash; int r; @@ -104,8 +104,7 @@ static int specifier_last_component(char specifier, void *data, void *userdata, return 0; } -int install_full_printf(UnitFileInstallInfo *i, const char *format, char **ret) { - +int install_full_printf(const UnitFileInstallInfo *i, const char *format, char **ret) { /* This is similar to unit_full_printf() but does not support * anything path-related. * diff --git a/src/shared/install-printf.h b/src/shared/install-printf.h index 378b3bfd92..fa8ea7ef81 100644 --- a/src/shared/install-printf.h +++ b/src/shared/install-printf.h @@ -3,4 +3,4 @@ #include "install.h" -int install_full_printf(UnitFileInstallInfo *i, const char *format, char **ret); +int install_full_printf(const UnitFileInstallInfo *i, const char *format, char **ret); diff --git a/src/shared/specifier.c b/src/shared/specifier.c index 78dda05b48..b8f7537f7a 100644 --- a/src/shared/specifier.c +++ b/src/shared/specifier.c @@ -27,7 +27,7 @@ * and "%" used for escaping. */ #define POSSIBLE_SPECIFIERS ALPHANUMERICAL "%" -int specifier_printf(const char *text, const Specifier table[], void *userdata, char **_ret) { +int specifier_printf(const char *text, const Specifier table[], const void *userdata, char **_ret) { size_t l, allocated = 0; _cleanup_free_ char *ret = NULL; char *t; @@ -102,7 +102,7 @@ int specifier_printf(const char *text, const Specifier table[], void *userdata, /* Generic handler for simple string replacements */ -int specifier_string(char specifier, void *data, void *userdata, char **ret) { +int specifier_string(char specifier, const void *data, const void *userdata, char **ret) { char *n; n = strdup(strempty(data)); @@ -113,7 +113,7 @@ int specifier_string(char specifier, void *data, void *userdata, char **ret) { return 0; } -int specifier_machine_id(char specifier, void *data, void *userdata, char **ret) { +int specifier_machine_id(char specifier, const void *data, const void *userdata, char **ret) { sd_id128_t id; char *n; int r; @@ -130,7 +130,7 @@ int specifier_machine_id(char specifier, void *data, void *userdata, char **ret) return 0; } -int specifier_boot_id(char specifier, void *data, void *userdata, char **ret) { +int specifier_boot_id(char specifier, const void *data, const void *userdata, char **ret) { sd_id128_t id; char *n; int r; @@ -147,7 +147,7 @@ int specifier_boot_id(char specifier, void *data, void *userdata, char **ret) { return 0; } -int specifier_host_name(char specifier, void *data, void *userdata, char **ret) { +int specifier_host_name(char specifier, const void *data, const void *userdata, char **ret) { char *n; n = gethostname_malloc(); @@ -158,7 +158,7 @@ int specifier_host_name(char specifier, void *data, void *userdata, char **ret) return 0; } -int specifier_kernel_release(char specifier, void *data, void *userdata, char **ret) { +int specifier_kernel_release(char specifier, const void *data, const void *userdata, char **ret) { struct utsname uts; char *n; int r; @@ -175,7 +175,7 @@ int specifier_kernel_release(char specifier, void *data, void *userdata, char ** return 0; } -int specifier_group_name(char specifier, void *data, void *userdata, char **ret) { +int specifier_group_name(char specifier, const void *data, const void *userdata, char **ret) { char *t; t = gid_to_name(getgid()); @@ -186,14 +186,14 @@ int specifier_group_name(char specifier, void *data, void *userdata, char **ret) return 0; } -int specifier_group_id(char specifier, void *data, void *userdata, char **ret) { +int specifier_group_id(char specifier, const void *data, const void *userdata, char **ret) { if (asprintf(ret, UID_FMT, getgid()) < 0) return -ENOMEM; return 0; } -int specifier_user_name(char specifier, void *data, void *userdata, char **ret) { +int specifier_user_name(char specifier, const void *data, const void *userdata, char **ret) { char *t; /* If we are UID 0 (root), this will not result in NSS, otherwise it might. This is good, as we want to be able @@ -211,7 +211,7 @@ int specifier_user_name(char specifier, void *data, void *userdata, char **ret) return 0; } -int specifier_user_id(char specifier, void *data, void *userdata, char **ret) { +int specifier_user_id(char specifier, const void *data, const void *userdata, char **ret) { if (asprintf(ret, UID_FMT, getuid()) < 0) return -ENOMEM; @@ -219,7 +219,7 @@ int specifier_user_id(char specifier, void *data, void *userdata, char **ret) { return 0; } -int specifier_user_home(char specifier, void *data, void *userdata, char **ret) { +int specifier_user_home(char specifier, const void *data, const void *userdata, char **ret) { /* On PID 1 (which runs as root) this will not result in NSS, * which is good. See above */ @@ -227,7 +227,7 @@ int specifier_user_home(char specifier, void *data, void *userdata, char **ret) return get_home_dir(ret); } -int specifier_user_shell(char specifier, void *data, void *userdata, char **ret) { +int specifier_user_shell(char specifier, const void *data, const void *userdata, char **ret) { /* On PID 1 (which runs as root) this will not result in NSS, * which is good. See above */ @@ -235,7 +235,7 @@ int specifier_user_shell(char specifier, void *data, void *userdata, char **ret) return get_shell(ret); } -int specifier_tmp_dir(char specifier, void *data, void *userdata, char **ret) { +int specifier_tmp_dir(char specifier, const void *data, const void *userdata, char **ret) { const char *p; char *copy; int r; @@ -252,7 +252,7 @@ int specifier_tmp_dir(char specifier, void *data, void *userdata, char **ret) { return 0; } -int specifier_var_tmp_dir(char specifier, void *data, void *userdata, char **ret) { +int specifier_var_tmp_dir(char specifier, const void *data, const void *userdata, char **ret) { const char *p; char *copy; int r; diff --git a/src/shared/specifier.h b/src/shared/specifier.h index 722200b998..d0221ef714 100644 --- a/src/shared/specifier.h +++ b/src/shared/specifier.h @@ -3,32 +3,32 @@ #include "string-util.h" -typedef int (*SpecifierCallback)(char specifier, void *data, void *userdata, char **ret); +typedef int (*SpecifierCallback)(char specifier, const void *data, const void *userdata, char **ret); typedef struct Specifier { const char specifier; const SpecifierCallback lookup; - void *data; + const void *data; } Specifier; -int specifier_printf(const char *text, const Specifier table[], void *userdata, char **ret); +int specifier_printf(const char *text, const Specifier table[], const void *userdata, char **ret); -int specifier_string(char specifier, void *data, void *userdata, char **ret); +int specifier_string(char specifier, const void *data, const void *userdata, char **ret); -int specifier_machine_id(char specifier, void *data, void *userdata, char **ret); -int specifier_boot_id(char specifier, void *data, void *userdata, char **ret); -int specifier_host_name(char specifier, void *data, void *userdata, char **ret); -int specifier_kernel_release(char specifier, void *data, void *userdata, char **ret); +int specifier_machine_id(char specifier, const void *data, const void *userdata, char **ret); +int specifier_boot_id(char specifier, const void *data, const void *userdata, char **ret); +int specifier_host_name(char specifier, const void *data, const void *userdata, char **ret); +int specifier_kernel_release(char specifier, const void *data, const void *userdata, char **ret); -int specifier_group_name(char specifier, void *data, void *userdata, char **ret); -int specifier_group_id(char specifier, void *data, void *userdata, char **ret); -int specifier_user_name(char specifier, void *data, void *userdata, char **ret); -int specifier_user_id(char specifier, void *data, void *userdata, char **ret); -int specifier_user_home(char specifier, void *data, void *userdata, char **ret); -int specifier_user_shell(char specifier, void *data, void *userdata, char **ret); +int specifier_group_name(char specifier, const void *data, const void *userdata, char **ret); +int specifier_group_id(char specifier, const void *data, const void *userdata, char **ret); +int specifier_user_name(char specifier, const void *data, const void *userdata, char **ret); +int specifier_user_id(char specifier, const void *data, const void *userdata, char **ret); +int specifier_user_home(char specifier, const void *data, const void *userdata, char **ret); +int specifier_user_shell(char specifier, const void *data, const void *userdata, char **ret); -int specifier_tmp_dir(char specifier, void *data, void *userdata, char **ret); -int specifier_var_tmp_dir(char specifier, void *data, void *userdata, char **ret); +int specifier_tmp_dir(char specifier, const void *data, const void *userdata, char **ret); +int specifier_var_tmp_dir(char specifier, const void *data, const void *userdata, char **ret); static inline char* specifier_escape(const char *string) { return strreplace(string, "%", "%%"); diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 08c57a3fb1..19225f8cfd 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -180,8 +180,8 @@ STATIC_DESTRUCTOR_REGISTER(arg_include_prefixes, freep); STATIC_DESTRUCTOR_REGISTER(arg_exclude_prefixes, freep); STATIC_DESTRUCTOR_REGISTER(arg_root, freep); -static int specifier_machine_id_safe(char specifier, void *data, void *userdata, char **ret); -static int specifier_directory(char specifier, void *data, void *userdata, char **ret); +static int specifier_machine_id_safe(char specifier, const void *data, const void *userdata, char **ret); +static int specifier_directory(char specifier, const void *data, const void *userdata, char **ret); static const Specifier specifier_table[] = { { 'm', specifier_machine_id_safe, NULL }, @@ -204,7 +204,7 @@ static const Specifier specifier_table[] = { {} }; -static int specifier_machine_id_safe(char specifier, void *data, void *userdata, char **ret) { +static int specifier_machine_id_safe(char specifier, const void *data, const void *userdata, char **ret) { int r; /* If /etc/machine_id is missing or empty (e.g. in a chroot environment) @@ -218,7 +218,7 @@ static int specifier_machine_id_safe(char specifier, void *data, void *userdata, return r; } -static int specifier_directory(char specifier, void *data, void *userdata, char **ret) { +static int specifier_directory(char specifier, const void *data, const void *userdata, char **ret) { struct table_entry { uint64_t type; const char *suffix; From 26526f9826a4aef2e83b6dd74cbb1f47cd8fdec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 12 Dec 2018 16:50:29 +0100 Subject: [PATCH 2/5] shared/install: mark UnitFileInstallInfo* as const where appropriate --- src/shared/install.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 822645ef43..de8044b28f 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -68,7 +68,7 @@ typedef struct { size_t n_rules; } Presets; -static inline bool unit_file_install_info_has_rules(UnitFileInstallInfo *i) { +static inline bool unit_file_install_info_has_rules(const UnitFileInstallInfo *i) { assert(i); return !strv_isempty(i->aliases) || @@ -76,7 +76,7 @@ static inline bool unit_file_install_info_has_rules(UnitFileInstallInfo *i) { !strv_isempty(i->required_by); } -static inline bool unit_file_install_info_has_also(UnitFileInstallInfo *i) { +static inline bool unit_file_install_info_has_also(const UnitFileInstallInfo *i) { assert(i); return !strv_isempty(i->also); @@ -711,7 +711,7 @@ static int is_symlink_with_known_name(const UnitFileInstallInfo *i, const char * static int find_symlinks_fd( const char *root_dir, - UnitFileInstallInfo *i, + const UnitFileInstallInfo *i, bool match_aliases, int fd, const char *path, @@ -845,7 +845,7 @@ static int find_symlinks_fd( static int find_symlinks( const char *root_dir, - UnitFileInstallInfo *i, + const UnitFileInstallInfo *i, bool match_name, const char *config_path, bool *same_name_link) { @@ -871,7 +871,7 @@ static int find_symlinks( static int find_symlinks_in_scope( UnitFileScope scope, const LookupPaths *paths, - UnitFileInstallInfo *i, + const UnitFileInstallInfo *i, bool match_name, UnitFileState *state) { @@ -987,7 +987,7 @@ static UnitFileInstallInfo *install_info_find(InstallContext *c, const char *nam } static int install_info_may_process( - UnitFileInstallInfo *i, + const UnitFileInstallInfo *i, const LookupPaths *paths, UnitFileChange **changes, size_t *n_changes) { From 2eded6cb2c5a7aebfafc77f9a23c1f6900da362c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Dec 2018 00:57:16 +0100 Subject: [PATCH 3/5] shared/install: remove two conditionals which are always false MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The name argument in UnitFileInstallInfo (i->name) should always be a unit file name, so the conditional always takes the 'else' branch. The only call chain that links to find_symlinks_fd() is unit_file_lookup_state → find_symlinks_in_scope → find_symlinks → find_symlinks_fd. But unit_file_lookup_state calls unit_name_is_valid(name), and then name is used to construct the UnitFileInstallInfo object in install_info_discover, which just uses the name it was given. --- src/shared/install.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index de8044b28f..ebc56a4aa6 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -794,23 +794,15 @@ static int find_symlinks_fd( if (!x) return -ENOMEM; - free(dest); - dest = x; + free_and_replace(dest, x); } - /* Check if the symlink itself matches what we - * are looking for */ - if (path_is_absolute(i->name)) - found_path = path_equal(p, i->name); - else - found_path = streq(de->d_name, i->name); + /* Check if the symlink itself matches what we are looking for */ + assert(unit_name_is_valid(i->name, UNIT_NAME_ANY)); + 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(i->name)) - found_dest = path_equal(dest, i->name); - else - found_dest = streq(basename(dest), i->name); + /* Check if what the symlink points to matches what we are looking for */ + found_dest = streq(basename(dest), i->name); if (found_path && found_dest) { _cleanup_free_ char *t = NULL; From 3e8d06d951d33c463e1efb67501e664f82cf3708 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Dec 2018 08:40:38 +0100 Subject: [PATCH 4/5] shared/install: add some more debugging info Just to make it easier to understand what is going on. --- src/shared/install.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index ebc56a4aa6..37d8830781 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -477,8 +477,10 @@ static int create_symlink( if (!dirname) return -ENOMEM; - if (chroot_symlinks_same(paths->root_dir, dirname, dest, old_path)) + if (chroot_symlinks_same(paths->root_dir, dirname, dest, old_path)) { + log_debug("Symlink %s → %s already exists", new_path, dest); return 1; + } if (!force) { unit_file_changes_add(changes, n_changes, -EEXIST, new_path, dest); @@ -2657,7 +2659,11 @@ int unit_file_lookup_state( r = install_info_discover(scope, &c, paths, name, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, &i, NULL, NULL); if (r < 0) - return r; + return log_debug_errno(r, "Failed to discover unit %s: %m", name); + + assert(IN_SET(i->type, UNIT_FILE_TYPE_REGULAR, UNIT_FILE_TYPE_MASKED)); + log_debug("Found unit %s at %s (%s)", name, strna(i->path), + i->type == UNIT_FILE_TYPE_REGULAR ? "regular file" : "mask"); /* Shortcut things, if the caller just wants to know if this unit exists. */ if (!ret) From 4b37c89f066209490197f680a597af2297ec008c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Dec 2018 10:46:27 +0100 Subject: [PATCH 5/5] shared/install: ignore symlinks which have lower priority than the unit file In #10583, a unit file lives in ~/.config/systemd/user, and 'systemctl --runtime --user mask' is used to create a symlink in /run. This symlink has lower priority than the config file, so 'systemctl --user' will happily load the unit file, and does't care about the symlink at all. But when asked if the unit is enabled, we'd look for all symlinks, find the symlink in the runtime directory, and report that the unit is runtime-enabled. In this particular case the fact that the symlink points at /dev/null, creates additional confusion, but it doesn't really matter: *any* symlink (or regular file) that is lower in the priority order is "covered" by the unit fragment, and should be ignored. Fixes #10583. --- src/shared/install.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 37d8830781..164eea2bde 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -715,6 +715,7 @@ static int find_symlinks_fd( const char *root_dir, const UnitFileInstallInfo *i, bool match_aliases, + bool ignore_same_name, int fd, const char *path, const char *config_path, @@ -761,7 +762,7 @@ static int find_symlinks_fd( } /* This will close nfd, regardless whether it succeeds or not */ - q = find_symlinks_fd(root_dir, i, match_aliases, nfd, + q = find_symlinks_fd(root_dir, i, match_aliases, ignore_same_name, nfd, p, config_path, same_name_link); if (q > 0) return 1; @@ -770,7 +771,7 @@ static int find_symlinks_fd( } else if (de->d_type == DT_LNK) { _cleanup_free_ char *p = NULL, *dest = NULL; - bool found_path, found_dest, b = false; + bool found_path = false, found_dest, b = false; int q; /* Acquire symlink name */ @@ -799,9 +800,14 @@ static int find_symlinks_fd( free_and_replace(dest, x); } - /* Check if the symlink itself matches what we are looking for */ assert(unit_name_is_valid(i->name, UNIT_NAME_ANY)); - found_path = streq(de->d_name, i->name); + if (!ignore_same_name) + /* Check if the symlink itself matches what we are looking for. + * + * If ignore_same_name is specified, we are in one of the directories which + * have lower priority than the unit file, and even if a file or symlink with + * this name was found, we should ignore it. */ + found_path = streq(de->d_name, i->name); /* Check if what the symlink points to matches what we are looking for */ found_dest = streq(basename(dest), i->name); @@ -841,6 +847,7 @@ static int find_symlinks( const char *root_dir, const UnitFileInstallInfo *i, bool match_name, + bool ignore_same_name, const char *config_path, bool *same_name_link) { @@ -858,7 +865,7 @@ static int find_symlinks( } /* This takes possession of fd and closes it */ - return find_symlinks_fd(root_dir, i, match_name, fd, + return find_symlinks_fd(root_dir, i, match_name, ignore_same_name, fd, config_path, config_path, same_name_link); } @@ -871,16 +878,21 @@ static int find_symlinks_in_scope( bool same_name_link_runtime = false, same_name_link_config = false; bool enabled_in_runtime = false, enabled_at_all = false; + bool ignore_same_name = false; char **p; int r; assert(paths); assert(i); + /* As we iterate over the list of search paths in paths->search_path, we may encounter "same name" + * symlinks. The ones which are "below" (i.e. have lower priority) than the unit file itself are + * efectively masked, so we should ignore them. */ + STRV_FOREACH(p, paths->search_path) { bool same_name_link = false; - r = find_symlinks(paths->root_dir, i, match_name, *p, &same_name_link); + r = find_symlinks(paths->root_dir, i, match_name, ignore_same_name, *p, &same_name_link); if (r < 0) return r; if (r > 0) { @@ -917,6 +929,11 @@ static int find_symlinks_in_scope( same_name_link_runtime = true; } } + + /* Check if next iteration will be "below" the unit file (either a regular file + * or a symlink), and hence should be ignored */ + if (!ignore_same_name && path_startswith(i->path, *p)) + ignore_same_name = true; } if (enabled_in_runtime) {