From c45e7e0cbf1060d096680cd0c7fe1fce64a69999 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 27 Nov 2018 17:05:27 +0100 Subject: [PATCH 1/9] systemctl: rename unit_file_find_path()'s return paramete to indicate that it is one --- src/systemctl/systemctl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 38545e8f5e..328c11a2cd 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2415,7 +2415,7 @@ static void warn_unit_file_changed(const char *name) { arg_scope == UNIT_FILE_SYSTEM ? "" : " --user"); } -static int unit_file_find_path(LookupPaths *lp, const char *unit_name, char **unit_path) { +static int unit_file_find_path(LookupPaths *lp, const char *unit_name, char **ret_unit_path) { char **p; assert(lp); @@ -2437,8 +2437,8 @@ static int unit_file_find_path(LookupPaths *lp, const char *unit_name, char **un if (r < 0) return log_error_errno(r, "Failed to access path \"%s\": %m", path); - if (unit_path) - *unit_path = TAKE_PTR(lpath); + if (ret_unit_path) + *ret_unit_path = TAKE_PTR(lpath); return 1; } From f67cb270606c46b3d5700dbea9cde2bc8ff0df63 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 11:10:37 +0100 Subject: [PATCH 2/9] systemctl: make sure we initialize return parameters in unit_file_find_path() on success According to our coding style return values should be initialized when we return any form of success, do so here too. --- src/systemctl/systemctl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 328c11a2cd..490b739f9e 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2443,6 +2443,9 @@ static int unit_file_find_path(LookupPaths *lp, const char *unit_name, char **re return 1; } + if (ret_unit_path) + *ret_unit_path = NULL; + return 0; } From b03677e2da759c1f99aedb7766bf086c484b33ab Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 27 Nov 2018 17:06:03 +0100 Subject: [PATCH 3/9] systemctl: rework unit_find_template_path() to follow coding style This makes sure that we don't clobber return values on failure and reset all return values on success. --- src/systemctl/systemctl.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 490b739f9e..ac55ec6c8b 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2452,30 +2452,43 @@ static int unit_file_find_path(LookupPaths *lp, const char *unit_name, char **re static int unit_find_template_path( const char *unit_name, LookupPaths *lp, - char **fragment_path, - char **template) { + char **ret_fragment_path, + char **ret_template) { - _cleanup_free_ char *_template = NULL; + _cleanup_free_ char *t = NULL, *f = NULL; int r; /* Returns 1 if a fragment was found, 0 if not found, negative on error. */ - r = unit_file_find_path(lp, unit_name, fragment_path); - if (r != 0) - return r; /* error or found a real unit */ + r = unit_file_find_path(lp, unit_name, &f); + if (r < 0) + return r; + if (r > 0) { + if (ret_fragment_path) + *ret_fragment_path = TAKE_PTR(f); + if (ret_template) + *ret_template = NULL; + return r; /* found a real unit */ + } + + r = unit_name_template(unit_name, &t); + if (r == -EINVAL) { + if (ret_fragment_path) + *ret_fragment_path = NULL; + if (ret_template) + *ret_template = NULL; - r = unit_name_template(unit_name, &_template); - if (r == -EINVAL) return 0; /* not a template, does not exist */ + } if (r < 0) return log_error_errno(r, "Failed to determine template name: %m"); - r = unit_file_find_path(lp, _template, fragment_path); + r = unit_file_find_path(lp, t, ret_fragment_path); if (r < 0) return r; - if (template) - *template = TAKE_PTR(_template); + if (ret_template) + *ret_template = r > 0 ? TAKE_PTR(t) : NULL; return r; } From 919d27208525551298b7536bcf9cc9a99614f70a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 27 Nov 2018 17:07:32 +0100 Subject: [PATCH 4/9] systemctl: rework error paths in unit_file_create_new() Let's use _cleanup_ to clean up stuff for us. --- src/systemctl/systemctl.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index ac55ec6c8b..d8e87ead67 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -6797,7 +6797,8 @@ static int unit_file_create_new( char **ret_new_path, char **ret_tmp_path) { - char *tmp_new_path, *tmp_tmp_path, *ending; + _cleanup_free_ char *new_path = NULL, *tmp_path = NULL; + const char *ending; int r; assert(unit_name); @@ -6805,18 +6806,16 @@ static int unit_file_create_new( assert(ret_tmp_path); ending = strjoina(unit_name, suffix); - r = get_file_to_edit(paths, ending, &tmp_new_path); + r = get_file_to_edit(paths, ending, &new_path); if (r < 0) return r; - r = create_edit_temp_file(tmp_new_path, tmp_new_path, &tmp_tmp_path); - if (r < 0) { - free(tmp_new_path); + r = create_edit_temp_file(new_path, new_path, &tmp_path); + if (r < 0) return r; - } - *ret_new_path = tmp_new_path; - *ret_tmp_path = tmp_tmp_path; + *ret_new_path = TAKE_PTR(new_path); + *ret_tmp_path = TAKE_PTR(tmp_path); return 0; } From 92d6b0bfdc19bb7ccd9f97e7fc4870bb2d276b68 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 27 Nov 2018 17:08:30 +0100 Subject: [PATCH 5/9] systemctl: use _cleanup_ logic for error paths in unit_file_create_copy(), too --- src/systemctl/systemctl.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index d8e87ead67..98beb80bfd 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -6827,7 +6827,7 @@ static int unit_file_create_copy( char **ret_new_path, char **ret_tmp_path) { - char *tmp_new_path, *tmp_tmp_path; + _cleanup_free_ char *new_path = NULL, *tmp_path = NULL; int r; assert(fragment_path); @@ -6835,33 +6835,28 @@ static int unit_file_create_copy( assert(ret_new_path); assert(ret_tmp_path); - r = get_file_to_edit(paths, unit_name, &tmp_new_path); + r = get_file_to_edit(paths, unit_name, &new_path); if (r < 0) return r; - if (!path_equal(fragment_path, tmp_new_path) && access(tmp_new_path, F_OK) == 0) { + if (!path_equal(fragment_path, new_path) && access(new_path, F_OK) >= 0) { char response; - r = ask_char(&response, "yn", "\"%s\" already exists. Overwrite with \"%s\"? [(y)es, (n)o] ", tmp_new_path, fragment_path); - if (r < 0) { - free(tmp_new_path); + r = ask_char(&response, "yn", "\"%s\" already exists. Overwrite with \"%s\"? [(y)es, (n)o] ", new_path, fragment_path); + if (r < 0) return r; - } if (response != 'y') { log_warning("%s ignored", unit_name); - free(tmp_new_path); return -EKEYREJECTED; } } - r = create_edit_temp_file(tmp_new_path, fragment_path, &tmp_tmp_path); - if (r < 0) { - free(tmp_new_path); + r = create_edit_temp_file(new_path, fragment_path, &tmp_path); + if (r < 0) return r; - } - *ret_new_path = tmp_new_path; - *ret_tmp_path = tmp_tmp_path; + *ret_new_path = TAKE_PTR(new_path); + *ret_tmp_path = TAKE_PTR(tmp_path); return 0; } From 5daacba2331f160cf793cb24dea3a1c868f5dafd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 11:10:24 +0100 Subject: [PATCH 6/9] systemctl: improve message when we skip a unit for editing a bit --- src/systemctl/systemctl.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 98beb80bfd..b823d5d116 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -6845,10 +6845,8 @@ static int unit_file_create_copy( r = ask_char(&response, "yn", "\"%s\" already exists. Overwrite with \"%s\"? [(y)es, (n)o] ", new_path, fragment_path); if (r < 0) return r; - if (response != 'y') { - log_warning("%s ignored", unit_name); - return -EKEYREJECTED; - } + if (response != 'y') + return log_warning_errno(SYNTHETIC_ERRNO(EKEYREJECTED), "%s skipped.", unit_name); } r = create_edit_temp_file(new_path, fragment_path, &tmp_path); From df79fdab214afce1ae53b8e324edd6402db2ef66 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 27 Nov 2018 17:09:25 +0100 Subject: [PATCH 7/9] systemctl: shorten code a bit --- src/systemctl/systemctl.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index b823d5d116..e7cf33ec52 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -6901,10 +6901,8 @@ static int run_editor(char **paths) { args[i] = editor_args[i]; } - STRV_FOREACH_PAIR(original_path, tmp_path, paths) { - args[i] = *tmp_path; - i++; - } + STRV_FOREACH_PAIR(original_path, tmp_path, paths) + args[i++] = *tmp_path; args[i] = NULL; if (n_editor_args > 0) From 85163756da6c559c9dc8ba3e035ecb587b2d48ef Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 27 Nov 2018 17:10:47 +0100 Subject: [PATCH 8/9] systemctl: rework message suggesting how to create a new unit file We need to specifiy --full for creating full unit files. Also, this is an explanatory hint, hence shouldn't be logged on LOG_ERR level. --- src/systemctl/systemctl.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index e7cf33ec52..0fcf597767 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -6952,10 +6952,10 @@ static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { assert(!path); if (!arg_force) { - log_error("Run 'systemctl edit%s --force %s' to create a new unit.", - arg_scope == UNIT_FILE_GLOBAL ? " --global" : - arg_scope == UNIT_FILE_USER ? " --user" : "", - *name); + log_info("Run 'systemctl edit%s --force --full %s' to create a new unit.", + arg_scope == UNIT_FILE_GLOBAL ? " --global" : + arg_scope == UNIT_FILE_USER ? " --user" : "", + *name); return -ENOENT; } From 02a126a33dc1709dfee2907f150a037787e61b99 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 27 Nov 2018 17:26:53 +0100 Subject: [PATCH 9/9] systemctl: if service manager couldn't load unit file, don't rely on it to tell us the fragment path Previously, "systemctl edit" exclusively used the service manager's per-unit FragmentPath property to figure out which file to edit, when operating on a non-template unit. If for some reason loading the unit file failed entirely though (LoadState=error), then FragmentPath would be empty, and thus the unit not editable. Let's fix this, by falling back to client-side unit file searching in this case. (Also, various other clean-ups to make the relevant functions follow our coding style) Fixes: #9561 --- src/systemctl/systemctl.c | 99 ++++++++++++++++++++++++++++----------- 1 file changed, 72 insertions(+), 27 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 0fcf597767..effbd5c07c 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2497,33 +2497,59 @@ static int unit_find_paths( sd_bus *bus, const char *unit_name, LookupPaths *lp, - char **fragment_path, - char ***dropin_paths) { + bool force_client_side, + char **ret_fragment_path, + char ***ret_dropin_paths) { - _cleanup_free_ char *path = NULL; _cleanup_strv_free_ char **dropins = NULL; + _cleanup_free_ char *path = NULL; int r; /** - * Finds where the unit is defined on disk. Returns 0 if the unit - * is not found. Returns 1 if it is found, and sets - * - the path to the unit in *path, if it exists on disk, - * - and a strv of existing drop-ins in *dropins, - * if the arg is not NULL and any dropins were found. + * Finds where the unit is defined on disk. Returns 0 if the unit is not found. Returns 1 if it is found, and + * sets: + * - the path to the unit in *ret_frament_path, if it exists on disk, + * - and a strv of existing drop-ins in *ret_dropin_paths, if the arg is not NULL and any dropins were found. + * + * Returns -ERFKILL if the unit is masked, and -EKEYREJECTED if the unit file could not be loaded for some + * reason (the latter only applies if we are going through the service manager) */ assert(unit_name); - assert(fragment_path); + assert(ret_fragment_path); assert(lp); - if (!install_client_side() && !unit_name_is_valid(unit_name, UNIT_NAME_TEMPLATE)) { + /* Go via the bus to acquire the path, unless we are explicitly told not to, or when the unit name is a template */ + if (!force_client_side && + !install_client_side() && + !unit_name_is_valid(unit_name, UNIT_NAME_TEMPLATE)) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - _cleanup_free_ char *unit = NULL; + _cleanup_free_ char *load_state = NULL, *unit = NULL; unit = unit_dbus_path_from_name(unit_name); if (!unit) return log_oom(); + r = sd_bus_get_property_string( + bus, + "org.freedesktop.systemd1", + unit, + "org.freedesktop.systemd1.Unit", + "LoadState", + &error, + &load_state); + if (r < 0) + return log_error_errno(r, "Failed to get LoadState: %s", bus_error_message(&error, r)); + + if (streq(load_state, "masked")) + return -ERFKILL; + if (streq(load_state, "not-found")) { + r = 0; + goto not_found; + } + if (!streq(load_state, "loaded")) + return -EKEYREJECTED; + r = sd_bus_get_property_string( bus, "org.freedesktop.systemd1", @@ -2535,7 +2561,7 @@ static int unit_find_paths( if (r < 0) return log_error_errno(r, "Failed to get FragmentPath: %s", bus_error_message(&error, r)); - if (dropin_paths) { + if (ret_dropin_paths) { r = sd_bus_get_property_strv( bus, "org.freedesktop.systemd1", @@ -2558,7 +2584,6 @@ static int unit_find_paths( r = unit_find_template_path(unit_name, lp, &path, &template); if (r < 0) return r; - if (r > 0) { if (null_or_empty_path(path)) /* The template is masked. Let's cut the process short. */ @@ -2582,25 +2607,29 @@ static int unit_find_paths( if (r < 0) return log_error_errno(r, "Failed to add unit name: %m"); - if (dropin_paths) { - r = unit_file_find_dropin_conf_paths(arg_root, lp->search_path, - NULL, names, &dropins); + if (ret_dropin_paths) { + r = unit_file_find_dropin_conf_paths(arg_root, lp->search_path, NULL, names, &dropins); if (r < 0) return r; } } - r = 0; + r = 0; if (!isempty(path)) { - *fragment_path = TAKE_PTR(path); + *ret_fragment_path = TAKE_PTR(path); r = 1; + } else + *ret_fragment_path = NULL; + + if (ret_dropin_paths) { + if (!strv_isempty(dropins)) { + *ret_dropin_paths = TAKE_PTR(dropins); + r = 1; + } else + *ret_dropin_paths = NULL; } - if (dropin_paths && !strv_isempty(dropins)) { - *dropin_paths = TAKE_PTR(dropins); - r = 1; - } not_found: if (r == 0 && !arg_force) log_error("No files found for %s.", unit_name); @@ -5390,9 +5419,16 @@ static int cat(int argc, char *argv[], void *userdata) { _cleanup_free_ char *fragment_path = NULL; _cleanup_strv_free_ char **dropin_paths = NULL; - r = unit_find_paths(bus, *name, &lp, &fragment_path, &dropin_paths); + r = unit_find_paths(bus, *name, &lp, false, &fragment_path, &dropin_paths); if (r == -ERFKILL) { - printf("%s# unit %s is masked%s\n", + printf("%s# Unit %s is masked%s.\n", + ansi_highlight_magenta(), + *name, + ansi_normal()); + continue; + } + if (r == -EKEYREJECTED) { + printf("%s# Unit %s could not be loaded.%s\n", ansi_highlight_magenta(), *name, ansi_normal()); @@ -5400,7 +5436,7 @@ static int cat(int argc, char *argv[], void *userdata) { } if (r < 0) return r; - else if (r == 0) + if (r == 0) return -ENOENT; if (first) @@ -6893,6 +6929,7 @@ static int run_editor(char **paths) { n_editor_args = strv_length(editor_args); argc += n_editor_args - 1; } + args = newa(const char*, argc + 1); if (n_editor_args > 0) { @@ -6944,7 +6981,15 @@ static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { _cleanup_free_ char *path = NULL, *new_path = NULL, *tmp_path = NULL, *tmp_name = NULL; const char *unit_name; - r = unit_find_paths(bus, *name, &lp, &path, NULL); + r = unit_find_paths(bus, *name, &lp, false, &path, NULL); + if (r == -EKEYREJECTED) { + /* If loading of the unit failed server side complete, then the server won't tell us the unit + * file path. In that case, find the file client side. */ + log_debug_errno(r, "Unit '%s' was not loaded correctly, retrying client-side.", *name); + r = unit_find_paths(bus, *name, &lp, true, &path, NULL); + } + if (r == -ERFKILL) + return log_error_errno(r, "Unit '%s' masked, cannot edit.", *name); if (r < 0) return r; @@ -6995,6 +7040,7 @@ static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { r = strv_push_pair(paths, new_path, tmp_path); if (r < 0) return log_oom(); + new_path = tmp_path = NULL; } @@ -7035,7 +7081,6 @@ static int edit(int argc, char *argv[], void *userdata) { r = unit_is_masked(bus, &lp, *tmp); if (r < 0) return r; - if (r > 0) { log_error("Cannot edit %s: unit is masked.", *tmp); return -EINVAL;