From 8eda5560b0613ad6de0a378ecfcadb735da33a06 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 16:22:41 +0100 Subject: [PATCH 1/6] systemctl: simplify trim_edit_markers() This is not performance sensitive, don#t try to be smart with realloc() Follow-up for: #25305 Fixes: #25303 --- src/systemctl/systemctl-edit.c | 60 ++++++++++++++-------------------- 1 file changed, 24 insertions(+), 36 deletions(-) diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index e8e6eeda29..8a424c9f68 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -459,51 +459,42 @@ static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { } static int trim_edit_markers(const char *path) { - _cleanup_free_ char *contents = NULL; - char *contents_start = NULL; - const char *contents_end = NULL; - size_t size; + _cleanup_free_ char *old_contents = NULL, *new_contents = NULL; + char *contents_start, *contents_end; + const char *c = NULL; int r; /* Trim out the lines between the two markers */ - r = read_full_file(path, &contents, NULL); + r = read_full_file(path, &old_contents, NULL); if (r < 0) return log_error_errno(r, "Failed to read temporary file \"%s\": %m", path); - size = strlen(contents); - - contents_start = strstr(contents, EDIT_MARKER_START); + contents_start = strstr(old_contents, EDIT_MARKER_START); if (contents_start) contents_start += strlen(EDIT_MARKER_START); else - contents_start = contents; + contents_start = old_contents; contents_end = strstr(contents_start, EDIT_MARKER_END); if (contents_end) - strshorten(contents_start, contents_end - contents_start); + contents_end[0] = 0; - contents_start = strstrip(contents_start); - if (*contents_start && !endswith(contents_start, "\n")) { - char *tmp = contents_start; - if (MALLOC_SIZEOF_SAFE(contents) - (contents_start - contents) - strlen(contents_start) < 2) { - if ((tmp = realloc(contents, size + 1))) { - contents_start = tmp + (contents_start - contents); - contents = tmp; - } - } + c = strstrip(contents_start); + if (isempty(c)) + return 0; /* All gone now */ - if (tmp) - strcat(contents_start, "\n"); - } + new_contents = strjoin(c, "\n"); /* Trim prefix and suffix, but ensure suffixed by single newline */ + if (!new_contents) + return log_oom(); - /* Write new contents if the trimming actually changed anything */ - if (strlen(contents) != size) { - r = write_string_file(path, contents_start, WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_TRUNCATE | WRITE_STRING_FILE_AVOID_NEWLINE); - if (r < 0) - return log_error_errno(r, "Failed to modify temporary file \"%s\": %m", path); - } + if (streq(old_contents, new_contents)) /* Don't touch the file if the above didn't change a thing */ + return 1; /* Unchanged, but good */ - return 0; + r = write_string_file(path, new_contents, WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_TRUNCATE | WRITE_STRING_FILE_AVOID_NEWLINE); + if (r < 0) + return log_error_errno(r, "Failed to modify temporary file \"%s\": %m", path); + + return 1; /* Changed, but good */ } int verb_edit(int argc, char *argv[], void *userdata) { @@ -561,16 +552,13 @@ int verb_edit(int argc, char *argv[], void *userdata) { * modification. */ r = trim_edit_markers(*tmp); if (r < 0) + goto end; + if (r == 0) /* has no actual contents? then ignore it */ continue; - if (null_or_empty_path(*tmp)) { - log_warning("Editing \"%s\" canceled: temporary file is empty.", *original); - continue; - } - - r = rename(*tmp, *original); + r = RET_NERRNO(rename(*tmp, *original)); if (r < 0) { - r = log_error_errno(errno, "Failed to rename \"%s\" to \"%s\": %m", *tmp, *original); + log_error_errno(r, "Failed to rename \"%s\" to \"%s\": %m", *tmp, *original); goto end; } } From 2865507008a407dcf0d8116349978ea185457647 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 17:13:36 +0100 Subject: [PATCH 2/6] systemctl: minor modernizations/simplifications --- src/systemctl/systemctl-edit.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index 8a424c9f68..7d919b28e2 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -110,7 +110,12 @@ int verb_cat(int argc, char *argv[], void *userdata) { return rc; } -static int create_edit_temp_file(const char *new_path, const char *original_path, char ** const original_unit_paths, char **ret_tmp_fn) { +static int create_edit_temp_file( + const char *new_path, + const char *original_path, + char ** const original_unit_paths, + char **ret_tmp_fn) { + _cleanup_free_ char *t = NULL; int r; @@ -154,8 +159,7 @@ static int create_edit_temp_file(const char *new_path, const char *original_path if (!f) return log_error_errno(errno, "Failed to open \"%s\": %m", t); - r = fchmod(fileno(f), 0644); - if (r < 0) + if (fchmod(fileno(f), 0644) < 0) return log_error_errno(errno, "Failed to change mode of \"%s\": %m", t); r = read_full_file(new_path, &new_contents, NULL); @@ -200,7 +204,6 @@ static int create_edit_temp_file(const char *new_path, const char *original_path } *ret_tmp_fn = TAKE_PTR(t); - return 0; } @@ -222,9 +225,7 @@ static int get_file_to_edit( run = path_join(paths->runtime_config, name); if (!run) return log_oom(); - } - if (arg_runtime) { if (access(path, F_OK) >= 0) return log_error_errno(SYNTHETIC_ERRNO(EEXIST), "Refusing to create \"%s\" because it would be overridden by \"%s\" anyway.", From 0ce6f0a35a4ed6d187c86751d36b76ca0e515f7a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 18:46:01 +0100 Subject: [PATCH 3/6] systemctl: line break string where the newlines are --- src/systemctl/systemctl-edit.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index 7d919b28e2..19df478818 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -168,8 +168,10 @@ static int create_edit_temp_file( fprintf(f, "### Editing %s\n" - EDIT_MARKER_START - "\n\n%s%s\n" + EDIT_MARKER_START "\n" + "\n" + "%s%s" + "\n" EDIT_MARKER_END, new_path, strempty(new_contents), From d88e1e484a1a0f5cdb97979016154898336734ea Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 18:46:20 +0100 Subject: [PATCH 4/6] systemctl: stop using basename() at one more place --- src/systemctl/systemctl-edit.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index 19df478818..4c68ae26d7 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -392,7 +392,6 @@ static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { STRV_FOREACH(name, names) { _cleanup_free_ char *path = NULL, *new_path = NULL, *tmp_path = NULL, *tmp_name = NULL; _cleanup_strv_free_ char **unit_paths = NULL; - const char *unit_name; r = unit_find_paths(bus, *name, &lp, false, &cached_name_map, &cached_id_map, &path, &unit_paths); if (r == -EKEYREJECTED) { @@ -416,12 +415,16 @@ static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { } /* Create a new unit from scratch */ - unit_name = *name; - r = unit_file_create_new(&lp, unit_name, + r = unit_file_create_new(&lp, *name, arg_full ? NULL : ".d/override.conf", NULL, &new_path, &tmp_path); } else { - unit_name = basename(path); + _cleanup_free_ char *unit_name = NULL; + + r = path_extract_filename(path, &unit_name); + if (r < 0) + return log_error_errno(r, "Failed to extract unit name from path '%s': %m", path); + /* We follow unit aliases, but we need to propagate the instance */ if (unit_name_is_valid(*name, UNIT_NAME_INSTANCE) && unit_name_is_valid(unit_name, UNIT_NAME_TEMPLATE)) { From 1ae886fe28cb6f3c6a80bde4cf2a26435100a59d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 17:14:51 +0100 Subject: [PATCH 5/6] systemctl: if we edit a single file only, jump to the right line --- src/systemctl/systemctl-edit.c | 170 ++++++++++++++++++++++----------- 1 file changed, 115 insertions(+), 55 deletions(-) diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index 4c68ae26d7..233ca35d51 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -114,13 +114,16 @@ static int create_edit_temp_file( const char *new_path, const char *original_path, char ** const original_unit_paths, - char **ret_tmp_fn) { + char **ret_tmp_fn, + unsigned *ret_edit_line) { _cleanup_free_ char *t = NULL; + unsigned ln = 1; int r; assert(new_path); assert(ret_tmp_fn); + assert(ret_edit_line); r = tempfn_random(new_path, NULL, &t); if (r < 0) @@ -177,6 +180,8 @@ static int create_edit_temp_file( strempty(new_contents), new_contents && endswith(new_contents, "\n") ? "" : "\n"); + ln = 4; /* start editing at the contents */ + /* Add a comment with the contents of the original unit files */ STRV_FOREACH(path, original_unit_paths) { _cleanup_free_ char *contents = NULL; @@ -206,6 +211,8 @@ static int create_edit_temp_file( } *ret_tmp_fn = TAKE_PTR(t); + *ret_edit_line = ln; + return 0; } @@ -240,34 +247,53 @@ static int get_file_to_edit( return 0; } +typedef struct EditFile { + char *path; + char *tmp; + unsigned line; +} EditFile; + +static void edit_file_free_all(EditFile **f) { + if (!f || !*f) + return; + + for (EditFile *i = *f; i->path; i++) { + free(i->path); + free(i->tmp); + } + + free(*f); +} + static int unit_file_create_new( const LookupPaths *paths, const char *unit_name, const char *suffix, char ** const original_unit_paths, - char **ret_new_path, - char **ret_tmp_path) { + EditFile *ret_edit_file) { _cleanup_free_ char *new_path = NULL, *tmp_path = NULL; + unsigned edit_line; const char *ending; int r; assert(unit_name); - assert(ret_new_path); - assert(ret_tmp_path); + assert(ret_edit_file); ending = strjoina(unit_name, suffix); r = get_file_to_edit(paths, ending, &new_path); if (r < 0) return r; - r = create_edit_temp_file(new_path, NULL, original_unit_paths, &tmp_path); + r = create_edit_temp_file(new_path, NULL, original_unit_paths, &tmp_path, &edit_line); if (r < 0) return r; - *ret_new_path = TAKE_PTR(new_path); - *ret_tmp_path = TAKE_PTR(tmp_path); - + *ret_edit_file = (EditFile) { + .path = TAKE_PTR(new_path), + .tmp = TAKE_PTR(tmp_path), + .line = edit_line, + }; return 0; } @@ -275,16 +301,15 @@ static int unit_file_create_copy( const LookupPaths *paths, const char *unit_name, const char *fragment_path, - char **ret_new_path, - char **ret_tmp_path) { + EditFile *ret_edit_file) { _cleanup_free_ char *new_path = NULL, *tmp_path = NULL; + unsigned edit_line; int r; assert(fragment_path); assert(unit_name); - assert(ret_new_path); - assert(ret_tmp_path); + assert(ret_edit_file); r = get_file_to_edit(paths, unit_name, &new_path); if (r < 0) @@ -300,30 +325,30 @@ static int unit_file_create_copy( return log_warning_errno(SYNTHETIC_ERRNO(EKEYREJECTED), "%s skipped.", unit_name); } - r = create_edit_temp_file(new_path, fragment_path, NULL, &tmp_path); + r = create_edit_temp_file(new_path, fragment_path, NULL, &tmp_path, &edit_line); if (r < 0) return r; - *ret_new_path = TAKE_PTR(new_path); - *ret_tmp_path = TAKE_PTR(tmp_path); - + *ret_edit_file = (EditFile) { + .path = TAKE_PTR(new_path), + .tmp = TAKE_PTR(tmp_path), + .line = edit_line, + }; return 0; } -static int run_editor(char **paths) { +static int run_editor(const EditFile *files) { int r; - assert(paths); + assert(files); r = safe_fork("(editor)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG|FORK_WAIT, NULL); if (r < 0) return r; if (r == 0) { - char **editor_args = NULL; size_t n_editor_args = 0, i = 1, argc; - const char **args, *editor; - - argc = strv_length(paths)/2 + 1; + char **editor_args = NULL, **args; + const char *editor; /* SYSTEMD_EDITOR takes precedence over EDITOR which takes precedence over VISUAL. If * neither SYSTEMD_EDITOR nor EDITOR nor VISUAL are present, we try to execute well known @@ -334,17 +359,22 @@ static int run_editor(char **paths) { if (!editor) editor = getenv("VISUAL"); - if (!isempty(editor)) { + if (isempty(editor)) + argc = 1; + else { editor_args = strv_split(editor, WHITESPACE); if (!editor_args) { (void) log_oom(); _exit(EXIT_FAILURE); } n_editor_args = strv_length(editor_args); - argc += n_editor_args - 1; + argc = n_editor_args; } - args = newa(const char*, argc + 1); + for (const EditFile *f = files; f->path; f++) + argc += 2; + + args = newa(char*, argc + 1); if (n_editor_args > 0) { args[0] = editor_args[0]; @@ -352,15 +382,26 @@ static int run_editor(char **paths) { args[i] = editor_args[i]; } - STRV_FOREACH_PAIR(original_path, tmp_path, paths) - args[i++] = *tmp_path; + if (files[0].path && files[0].line > 1 && !files[1].path) { + /* If editing a single file only, use the +LINE syntax to put cursor on the right line */ + if (asprintf(args + i, "+%u", files[0].line) < 0) { + (void) log_oom(); + _exit(EXIT_FAILURE); + } + + i++; + args[i++] = files[0].tmp; + } else + for (const EditFile *f = files; f->path; f++) + args[i++] = f->tmp; + args[i] = NULL; if (n_editor_args > 0) execvp(args[0], (char* const*) args); FOREACH_STRING(name, "editor", "nano", "vim", "vi") { - args[0] = name; + args[0] = (char*) name; execvp(name, (char* const*) args); /* We do not fail if the editor doesn't exist because we want to try each one of them * before failing. */ @@ -377,20 +418,26 @@ static int run_editor(char **paths) { return 0; } -static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { +static int find_paths_to_edit( + sd_bus *bus, + char **names, + EditFile **ret_edit_files) { + _cleanup_(hashmap_freep) Hashmap *cached_name_map = NULL, *cached_id_map = NULL; + _cleanup_(edit_file_free_all) EditFile *edit_files = NULL; _cleanup_(lookup_paths_free) LookupPaths lp = {}; + size_t n_edit_files = 0; int r; assert(names); - assert(paths); + assert(ret_edit_files); r = lookup_paths_init(&lp, arg_scope, 0, arg_root); if (r < 0) return r; STRV_FOREACH(name, names) { - _cleanup_free_ char *path = NULL, *new_path = NULL, *tmp_path = NULL, *tmp_name = NULL; + _cleanup_free_ char *path = NULL, *tmp_name = NULL; _cleanup_strv_free_ char **unit_paths = NULL; r = unit_find_paths(bus, *name, &lp, false, &cached_name_map, &cached_id_map, &path, &unit_paths); @@ -405,6 +452,9 @@ static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { if (r < 0) return r; + if (!GREEDY_REALLOC0(edit_files, n_edit_files + 2)) + return log_oom(); + if (!path) { if (!arg_force) { log_info("Run 'systemctl edit%s --force --full %s' to create a new unit.", @@ -415,9 +465,12 @@ static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { } /* Create a new unit from scratch */ - r = unit_file_create_new(&lp, *name, - arg_full ? NULL : ".d/override.conf", - NULL, &new_path, &tmp_path); + r = unit_file_create_new( + &lp, + *name, + arg_full ? NULL : ".d/override.conf", + NULL, + edit_files + n_edit_files); } else { _cleanup_free_ char *unit_name = NULL; @@ -442,25 +495,31 @@ static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { } if (arg_full) - r = unit_file_create_copy(&lp, unit_name, path, &new_path, &tmp_path); + r = unit_file_create_copy( + &lp, + unit_name, + path, + edit_files + n_edit_files); else { r = strv_prepend(&unit_paths, path); if (r < 0) return log_oom(); - r = unit_file_create_new(&lp, unit_name, ".d/override.conf", unit_paths, &new_path, &tmp_path); + r = unit_file_create_new( + &lp, + unit_name, + ".d/override.conf", + unit_paths, + edit_files + n_edit_files); } } if (r < 0) return r; - r = strv_push_pair(paths, new_path, tmp_path); - if (r < 0) - return log_oom(); - - new_path = tmp_path = NULL; + n_edit_files++; } + *ret_edit_files = n_edit_files > 0 ? TAKE_PTR(edit_files) : NULL; return 0; } @@ -504,9 +563,9 @@ static int trim_edit_markers(const char *path) { } int verb_edit(int argc, char *argv[], void *userdata) { + _cleanup_(edit_file_free_all) EditFile *edit_files = NULL; _cleanup_(lookup_paths_free) LookupPaths lp = {}; _cleanup_strv_free_ char **names = NULL; - _cleanup_strv_free_ char **paths = NULL; sd_bus *bus; int r; @@ -542,31 +601,32 @@ int verb_edit(int argc, char *argv[], void *userdata) { return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot edit %s: unit is masked.", *tmp); } - r = find_paths_to_edit(bus, names, &paths); + r = find_paths_to_edit(bus, names, &edit_files); if (r < 0) return r; - - if (strv_isempty(paths)) + if (!edit_files) return -ENOENT; - r = run_editor(paths); + r = run_editor(edit_files); if (r < 0) goto end; - STRV_FOREACH_PAIR(original, tmp, paths) { + for (EditFile *f = edit_files; f->path; f++) { /* If the temporary file is empty we ignore it. This allows the user to cancel the * modification. */ - r = trim_edit_markers(*tmp); + r = trim_edit_markers(f->tmp); if (r < 0) goto end; if (r == 0) /* has no actual contents? then ignore it */ continue; - r = RET_NERRNO(rename(*tmp, *original)); + r = RET_NERRNO(rename(f->tmp, f->path)); if (r < 0) { - log_error_errno(r, "Failed to rename \"%s\" to \"%s\": %m", *tmp, *original); + log_error_errno(r, "Failed to rename \"%s\" to \"%s\": %m", f->tmp, f->path); goto end; } + + log_info("Successfully installed edited file '%s'.", f->path); } r = 0; @@ -578,16 +638,16 @@ int verb_edit(int argc, char *argv[], void *userdata) { } end: - STRV_FOREACH_PAIR(original, tmp, paths) { - (void) unlink(*tmp); + for (EditFile *f = edit_files; f && f->path; f++) { + (void) unlink(f->tmp); /* Removing empty dropin dirs */ if (!arg_full) { _cleanup_free_ char *dir = NULL; - r = path_extract_directory(*original, &dir); + r = path_extract_directory(f->path, &dir); if (r < 0) - return log_error_errno(r, "Failed to extract directory from '%s': %m", *original); + return log_error_errno(r, "Failed to extract directory from '%s': %m", f->path); /* No need to check if the dir is empty, rmdir does nothing if it is not the case. */ (void) rmdir(dir); From 557bde331b1df20e0bb0c7fc208db6653288b26e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 24 Nov 2022 14:59:25 +0100 Subject: [PATCH 6/6] systemctl: don't unlink non-existing temporary files --- src/systemctl/systemctl-edit.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index 233ca35d51..3d519ccb8d 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -626,6 +626,7 @@ int verb_edit(int argc, char *argv[], void *userdata) { goto end; } + f->tmp = mfree(f->tmp); log_info("Successfully installed edited file '%s'.", f->path); } @@ -638,8 +639,10 @@ int verb_edit(int argc, char *argv[], void *userdata) { } end: - for (EditFile *f = edit_files; f && f->path; f++) { - (void) unlink(f->tmp); + for (EditFile *f = ASSERT_PTR(edit_files); f->path; f++) { + + if (f->tmp) + (void) unlink(f->tmp); /* Removing empty dropin dirs */ if (!arg_full) {