From 7a2a7f2c00b1cbba2687bea359152a94565aad59 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 Mar 2023 17:18:29 +0900 Subject: [PATCH 1/6] systemctl-edit: shorten code a bit --- src/systemctl/systemctl-edit.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index d85abbd165..ad8c96bd3a 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -160,11 +160,7 @@ static int unit_file_create_new( if (r < 0) return r; - r = edit_files_add(context, new_path, NULL, original_unit_paths); - if (r < 0) - return r; - - return 0; + return edit_files_add(context, new_path, NULL, original_unit_paths); } static int unit_file_create_copy( @@ -196,11 +192,7 @@ static int unit_file_create_copy( return log_warning_errno(SYNTHETIC_ERRNO(EKEYREJECTED), "%s skipped.", unit_name); } - r = edit_files_add(context, new_path, fragment_path, NULL); - if (r < 0) - return r; - - return 0; + return edit_files_add(context, new_path, fragment_path, NULL); } static int find_paths_to_edit( From 321291d82bd36834308aa99dffe4b62d1c4f68b2 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 Mar 2023 17:19:35 +0900 Subject: [PATCH 2/6] systemctl-edit: invert one error check --- src/systemctl/systemctl-edit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index ad8c96bd3a..5f42dc239f 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -367,9 +367,9 @@ int verb_edit(int argc, char *argv[], void *userdata) { if (!arg_no_reload && !install_client_side()) { r = daemon_reload(ACTION_RELOAD, /* graceful= */ false); - if (r > 0) - r = 0; + if (r < 0) + return r; } - return r; + return 0; } From 043db3400ce24335293d9424dc41c2457660c0c8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 Mar 2023 17:42:34 +0900 Subject: [PATCH 3/6] edit-util: make EditFile take reference of EditFileContext No functional change, just preparation for later commits. --- src/shared/edit-util.c | 1 + src/shared/edit-util.h | 12 ++++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/shared/edit-util.c b/src/shared/edit-util.c index 822175a472..212afb9fd5 100644 --- a/src/shared/edit-util.c +++ b/src/shared/edit-util.c @@ -95,6 +95,7 @@ int edit_files_add( } context->files[context->n_files] = (EditFile) { + .context = context, .path = TAKE_PTR(new_path), .original_path = TAKE_PTR(new_original_path), .comment_paths = TAKE_PTR(new_comment_paths), diff --git a/src/shared/edit-util.h b/src/shared/edit-util.h index 7b705060e4..8c838bcf95 100644 --- a/src/shared/edit-util.h +++ b/src/shared/edit-util.h @@ -3,21 +3,25 @@ #include "path-lookup.h" -typedef struct EditFile { +typedef struct EditFile EditFile; +typedef struct EditFileContext EditFileContext; + +struct EditFile { + EditFileContext *context; char *path; char *original_path; char **comment_paths; char *temp; unsigned line; -} EditFile; +}; -typedef struct EditFileContext { +struct EditFileContext { EditFile *files; size_t n_files; const char *marker_start; const char *marker_end; bool remove_parent; -} EditFileContext; +}; void edit_file_context_done(EditFileContext *context); From c629a5bc440e7852197ea0491a4da860b712836f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 Mar 2023 17:35:55 +0900 Subject: [PATCH 4/6] edit-util: make create_edit_temp_file() take EditFile as the argument No functional changes, just refactoring. --- src/shared/edit-util.c | 77 ++++++++++++++++++------------------------ 1 file changed, 32 insertions(+), 45 deletions(-) diff --git a/src/shared/edit-util.c b/src/shared/edit-util.c index 212afb9fd5..8884b62f53 100644 --- a/src/shared/edit-util.c +++ b/src/shared/edit-util.c @@ -105,37 +105,33 @@ int edit_files_add( return 1; } -static int create_edit_temp_file( - const char *target_path, - const char *original_path, - char * const *comment_paths, - const char *marker_start, - const char *marker_end, - char **ret_temp_filename, - unsigned *ret_edit_line) { - +static int create_edit_temp_file(EditFile *e) { _cleanup_free_ char *temp = NULL; unsigned line = 1; int r; - assert(target_path); - assert(!comment_paths || (marker_start && marker_end)); - assert(ret_temp_filename); + assert(e); + assert(e->context); + assert(e->path); + assert(!e->comment_paths || (e->context->marker_start && e->context->marker_end)); - r = tempfn_random(target_path, NULL, &temp); + if (e->temp) + return 0; + + r = tempfn_random(e->path, NULL, &temp); if (r < 0) - return log_error_errno(r, "Failed to determine temporary filename for \"%s\": %m", target_path); + return log_error_errno(r, "Failed to determine temporary filename for \"%s\": %m", e->path); - r = mkdir_parents_label(target_path, 0755); + r = mkdir_parents_label(e->path, 0755); if (r < 0) - return log_error_errno(r, "Failed to create parent directories for \"%s\": %m", target_path); + return log_error_errno(r, "Failed to create parent directories for \"%s\": %m", e->path); - if (original_path) { - r = mac_selinux_create_file_prepare(target_path, S_IFREG); + if (e->original_path) { + r = mac_selinux_create_file_prepare(e->path, S_IFREG); if (r < 0) return r; - r = copy_file(original_path, temp, 0, 0644, 0, 0, COPY_REFLINK); + r = copy_file(e->original_path, temp, 0, 0644, 0, 0, COPY_REFLINK); if (r == -ENOENT) { r = touch(temp); mac_selinux_create_file_clear(); @@ -144,15 +140,15 @@ static int create_edit_temp_file( } else { mac_selinux_create_file_clear(); if (r < 0) - return log_error_errno(r, "Failed to create temporary file for \"%s\": %m", target_path); + return log_error_errno(r, "Failed to create temporary file for \"%s\": %m", e->path); } } - if (comment_paths) { + if (e->comment_paths) { _cleanup_free_ char *target_contents = NULL; _cleanup_fclose_ FILE *f = NULL; - r = mac_selinux_create_file_prepare(target_path, S_IFREG); + r = mac_selinux_create_file_prepare(e->path, S_IFREG); if (r < 0) return r; @@ -164,9 +160,9 @@ static int create_edit_temp_file( if (fchmod(fileno(f), 0644) < 0) return log_error_errno(errno, "Failed to change mode of temporary file \"%s\": %m", temp); - r = read_full_file(target_path, &target_contents, NULL); + r = read_full_file(e->path, &target_contents, NULL); if (r < 0 && r != -ENOENT) - return log_error_errno(r, "Failed to read target file \"%s\": %m", target_path); + return log_error_errno(r, "Failed to read target file \"%s\": %m", e->path); fprintf(f, "### Editing %s\n" @@ -175,20 +171,20 @@ static int create_edit_temp_file( "%s%s" "\n" "%s\n", - target_path, - marker_start, + e->path, + e->context->marker_start, strempty(target_contents), target_contents && endswith(target_contents, "\n") ? "" : "\n", - marker_end); + e->context->marker_end); line = 4; /* Start editing at the contents area */ /* Add a comment with the contents of the original files */ - STRV_FOREACH(path, comment_paths) { + STRV_FOREACH(path, e->comment_paths) { _cleanup_free_ char *contents = NULL; /* Skip the file that's being edited, already processed in above */ - if (path_equal(*path, target_path)) + if (path_equal(*path, e->path)) continue; r = read_full_file(*path, &contents, NULL); @@ -212,10 +208,8 @@ static int create_edit_temp_file( return log_error_errno(r, "Failed to create temporary file \"%s\": %m", temp); } - *ret_temp_filename = TAKE_PTR(temp); - - if (ret_edit_line) - *ret_edit_line = line; + e->temp = TAKE_PTR(temp); + e->line = line; return 0; } @@ -347,18 +341,11 @@ int do_edit_files_and_install(EditFileContext *context) { if (context->n_files == 0) return log_debug_errno(SYNTHETIC_ERRNO(ENOENT), "Got no files to edit."); - FOREACH_ARRAY(i, context->files, context->n_files) - if (isempty(i->temp)) { - r = create_edit_temp_file(i->path, - i->original_path, - i->comment_paths, - context->marker_start, - context->marker_end, - &i->temp, - &i->line); - if (r < 0) - return r; - } + FOREACH_ARRAY(i, context->files, context->n_files) { + r = create_edit_temp_file(i); + if (r < 0) + return r; + } r = run_editor(context); if (r < 0) From aec8200b12bb8ebce2be87e0261e3a205eaa63d5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 Mar 2023 17:38:03 +0900 Subject: [PATCH 5/6] edit-util: fix potentical crash when no edit markers This also makes trim_edit_markers() take EditFile as the argument. --- src/shared/edit-util.c | 44 +++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/src/shared/edit-util.c b/src/shared/edit-util.c index 8884b62f53..207732e320 100644 --- a/src/shared/edit-util.c +++ b/src/shared/edit-util.c @@ -292,30 +292,38 @@ static int run_editor(const EditFileContext *context) { return 0; } -static int trim_edit_markers(const char *path, const char *marker_start, const char *marker_end) { +static int trim_edit_markers(EditFile *e) { _cleanup_free_ char *old_contents = NULL, *new_contents = NULL; - char *contents_start, *contents_end; - const char *c = NULL; + const char *c; int r; - assert(!marker_start == !marker_end); + assert(e); + assert(e->context); + assert(e->temp); /* Trim out the lines between the two markers */ - r = read_full_file(path, &old_contents, NULL); + r = read_full_file(e->temp, &old_contents, NULL); if (r < 0) - return log_error_errno(r, "Failed to read temporary file \"%s\": %m", path); + return log_error_errno(r, "Failed to read temporary file \"%s\": %m", e->temp); - contents_start = strstr(old_contents, marker_start); - if (contents_start) - contents_start += strlen(marker_start); - else - contents_start = old_contents; + if (e->context->marker_start) { + char *contents_start, *contents_end; - contents_end = strstr(contents_start, marker_end); - if (contents_end) - contents_end[0] = 0; + assert(e->context->marker_end); - c = strstrip(contents_start); + contents_start = strstr(old_contents, e->context->marker_start); + if (contents_start) + contents_start += strlen(e->context->marker_start); + else + contents_start = old_contents; + + contents_end = strstr(contents_start, e->context->marker_end); + if (contents_end) + contents_end[0] = 0; + + c = strstrip(contents_start); + } else + c = strstrip(old_contents); if (isempty(c)) return 0; /* All gone now */ @@ -326,9 +334,9 @@ static int trim_edit_markers(const char *path, const char *marker_start, const c if (streq(old_contents, new_contents)) /* Don't touch the file if the above didn't change a thing */ return 1; /* Unchanged, but good */ - r = write_string_file(path, new_contents, WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_TRUNCATE | WRITE_STRING_FILE_AVOID_NEWLINE); + r = write_string_file(e->temp, 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 log_error_errno(r, "Failed to modify temporary file \"%s\": %m", e->temp); return 1; /* Changed, but good */ } @@ -353,7 +361,7 @@ int do_edit_files_and_install(EditFileContext *context) { FOREACH_ARRAY(i, context->files, context->n_files) { /* Always call trim_edit_markers to tell if the temp file is empty */ - r = trim_edit_markers(i->temp, context->marker_start, context->marker_end); + r = trim_edit_markers(i); if (r < 0) return r; if (r == 0) /* temp file doesn't carry actual changes, ignoring */ From 0a742f3622531f4871506ff43e994c8dcf7c782d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 Mar 2023 22:20:51 +0900 Subject: [PATCH 6/6] edit-util: unlink temporary file on failure Addresses the suggestion https://github.com/systemd/systemd/pull/26756#discussion_r1133078705. --- src/shared/edit-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/edit-util.c b/src/shared/edit-util.c index 207732e320..ba1f4128a0 100644 --- a/src/shared/edit-util.c +++ b/src/shared/edit-util.c @@ -106,7 +106,7 @@ int edit_files_add( } static int create_edit_temp_file(EditFile *e) { - _cleanup_free_ char *temp = NULL; + _cleanup_(unlink_and_freep) char *temp = NULL; unsigned line = 1; int r;