From 7a729f876b2494e2f1d1def73b828a9b70dd4723 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 7 Jul 2024 17:21:08 +0200 Subject: [PATCH 1/4] edit-util: clean up run_editor() a bit - Add missing assertions - Close all fds before spawning editor - Use FOREACH_STRING() + empty_to_null() where appropriate Note that this slightly changes the behavior, in that empty envvars would be treated as unset and we'd try the next candidate. But the new behavior is better IMO. --- src/shared/edit-util.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/shared/edit-util.c b/src/shared/edit-util.c index cfb2828f4e3..2d0129eda27 100644 --- a/src/shared/edit-util.c +++ b/src/shared/edit-util.c @@ -236,23 +236,22 @@ static int run_editor_child(const EditFileContext *context) { const char *editor; int r; + assert(context); + assert(context->n_files >= 1); + /* 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 editors. */ - editor = getenv("SYSTEMD_EDITOR"); - if (!editor) - editor = getenv("EDITOR"); - if (!editor) - editor = getenv("VISUAL"); + FOREACH_STRING(e, "SYSTEMD_EDITOR", "EDITOR", "VISUAL") { + editor = empty_to_null(getenv(e)); + if (editor) + break; + } - if (!isempty(editor)) { - _cleanup_strv_free_ char **editor_args = NULL; - - editor_args = strv_split(editor, WHITESPACE); - if (!editor_args) + if (editor) { + args = strv_split(editor, WHITESPACE); + if (!args) return log_oom(); - - args = TAKE_PTR(editor_args); } if (context->n_files == 1 && context->files[0].line > 1) { @@ -268,7 +267,7 @@ static int run_editor_child(const EditFileContext *context) { return log_oom(); } - if (!isempty(editor)) + if (editor) execvp(args[0], (char* const*) args); bool prepended = false; @@ -298,7 +297,7 @@ static int run_editor(const EditFileContext *context) { assert(context); - r = safe_fork("(editor)", FORK_RESET_SIGNALS|FORK_DEATHSIG_SIGTERM|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG|FORK_WAIT, NULL); + r = safe_fork("(editor)", FORK_RESET_SIGNALS|FORK_DEATHSIG_SIGTERM|FORK_RLIMIT_NOFILE_SAFE|FORK_CLOSE_ALL_FDS|FORK_REOPEN_LOG|FORK_LOG|FORK_WAIT, NULL); if (r < 0) return r; if (r == 0) { /* Child */ From 3b5b2ff8fa6413afc2e5a058ea8281a58a4c691d Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 11 Aug 2024 15:41:30 +0200 Subject: [PATCH 2/4] edit-util: do not try to recreate temp file if missing We initially read from temp file, then strip it, and write back to it. If the file suddenly disappeared during the process, it indicates someone else is touching our temp file behind our back. Let's not silently continue. --- 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 2d0129eda27..b1f2aade275 100644 --- a/src/shared/edit-util.c +++ b/src/shared/edit-util.c @@ -356,7 +356,7 @@ static int strip_edit_temp_file(EditFile *e) { return 1; /* Contents have real changes */ r = write_string_file(e->temp, new_contents, - WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_TRUNCATE | WRITE_STRING_FILE_AVOID_NEWLINE); + WRITE_STRING_FILE_TRUNCATE | WRITE_STRING_FILE_AVOID_NEWLINE); if (r < 0) return log_error_errno(r, "Failed to strip temporary file '%s': %m", e->temp); From 40f5c372c22d2dea20c2d5155754faec9c4460cd Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 11 Aug 2024 15:41:07 +0200 Subject: [PATCH 3/4] edit-util: several cleanups for --stdin handling Follow-up for 329050c5e2c7e9561699f87b5edb72edd0d54c96 I don't particularly favor the duplicated strstrip() and such, so let's ensure if we get fixed data it's only trimmed once. Subsequently we can benefit more by making all copies reflinks. --- src/shared/edit-util.c | 122 +++++++++++++++++++++++++++++++---------- 1 file changed, 93 insertions(+), 29 deletions(-) diff --git a/src/shared/edit-util.c b/src/shared/edit-util.c index b1f2aade275..5dd8595a369 100644 --- a/src/shared/edit-util.c +++ b/src/shared/edit-util.c @@ -111,6 +111,9 @@ int edit_files_add( static int populate_edit_temp_file(EditFile *e, FILE *f, const char *filename) { assert(e); + assert(e->context); + assert(!e->context->stdin); + assert(e->path); assert(f); assert(filename); @@ -197,6 +200,7 @@ static int create_edit_temp_file(EditFile *e, const char *contents, size_t conte assert(e->path); assert(!e->comment_paths || (e->context->marker_start && e->context->marker_end)); assert(contents || contents_size == 0); + assert(e->context->stdin == !!contents); if (e->temp) return 0; @@ -215,7 +219,7 @@ static int create_edit_temp_file(EditFile *e, const char *contents, size_t conte if (e->context->stdin) { if (fwrite(contents, 1, contents_size, f) != contents_size) return log_error_errno(SYNTHETIC_ERRNO(EIO), - "Failed to copy input to temporary file '%s'.", temp); + "Failed to write stdin data to temporary file '%s'.", temp); } else { r = populate_edit_temp_file(e, f, temp); if (r < 0) @@ -341,11 +345,8 @@ static int strip_edit_temp_file(EditFile *e) { } else stripped = strstrip(tmp); - if (isempty(stripped)) { - /* File is empty (has no real changes) */ - log_notice("%s: after editing, new contents are empty, not writing file.", e->path); - return 0; - } + if (isempty(stripped)) + return 0; /* File is empty (has no real changes) */ /* Trim prefix and suffix, but ensure suffixed by single newline */ new_contents = strjoin(stripped, "\n"); @@ -363,9 +364,70 @@ static int strip_edit_temp_file(EditFile *e) { return 1; /* Contents have real changes */ } +static int edit_file_install_one(EditFile *e) { + int r; + + assert(e); + assert(e->path); + assert(e->temp); + + r = strip_edit_temp_file(e); + if (r <= 0) + return r; + + r = RET_NERRNO(rename(e->temp, e->path)); + if (r < 0) + return log_error_errno(r, + "Failed to rename temporary file '%s' to target file '%s': %m", + e->temp, e->path); + e->temp = mfree(e->temp); + + return 1; +} + +static int edit_file_install_one_stdin(EditFile *e, const char *contents, size_t contents_size, int *fd) { + int r; + + assert(e); + assert(e->path); + assert(contents || contents_size == 0); + assert(fd); + + if (contents_size == 0) + return 0; + + if (*fd >= 0) { + r = mkdir_parents_label(e->path, 0755); + if (r < 0) + return log_error_errno(r, "Failed to create parent directories for '%s': %m", e->path); + + r = copy_file_atomic_at(*fd, NULL, AT_FDCWD, e->path, 0644, COPY_REFLINK|COPY_REPLACE|COPY_MAC_CREATE); + if (r < 0) + return log_error_errno(r, "Failed to copy stdin contents to '%s': %m", e->path); + + return 1; + } + + r = create_edit_temp_file(e, contents, contents_size); + if (r < 0) + return r; + + _cleanup_close_ int tfd = open(e->temp, O_PATH|O_CLOEXEC); + if (tfd < 0) + return log_error_errno(errno, "Failed to pin temporary file '%s': %m", e->temp); + + r = edit_file_install_one(e); + if (r <= 0) + return r; + + *fd = TAKE_FD(tfd); + + return 1; +} + int do_edit_files_and_install(EditFileContext *context) { - _cleanup_free_ char *data = NULL; - size_t data_size = 0; + _cleanup_free_ char *stdin_data = NULL; + size_t stdin_size = 0; int r; assert(context); @@ -374,38 +436,40 @@ int do_edit_files_and_install(EditFileContext *context) { return log_debug_errno(SYNTHETIC_ERRNO(ENOENT), "Got no files to edit."); if (context->stdin) { - r = read_full_stream(stdin, &data, &data_size); + r = read_full_stream(stdin, &stdin_data, &stdin_size); if (r < 0) return log_error_errno(r, "Failed to read stdin: %m"); - } + } else { + FOREACH_ARRAY(editfile, context->files, context->n_files) { + r = create_edit_temp_file(editfile, /* contents = */ NULL, /* contents_size = */ 0); + if (r < 0) + return r; + } - FOREACH_ARRAY(editfile, context->files, context->n_files) { - r = create_edit_temp_file(editfile, data, data_size); - if (r < 0) - return r; - } - - if (!context->stdin) { r = run_editor(context); if (r < 0) return r; } + _cleanup_close_ int stdin_data_fd = -EBADF; + FOREACH_ARRAY(editfile, context->files, context->n_files) { - /* Always call strip_edit_temp_file which will tell if the temp file has actual changes */ - r = strip_edit_temp_file(editfile); + if (context->stdin) { + r = edit_file_install_one_stdin(editfile, stdin_data, stdin_size, &stdin_data_fd); + if (r == 0) { + log_notice("Stripped stdin content is empty, not writing file."); + return 0; + } + } else { + r = edit_file_install_one(editfile); + if (r == 0) { + log_notice("%s: after editing, new contents are empty, not writing file.", + editfile->path); + continue; + } + } if (r < 0) return r; - if (r == 0) /* temp file doesn't carry actual changes, ignoring */ - continue; - - r = RET_NERRNO(rename(editfile->temp, editfile->path)); - if (r < 0) - return log_error_errno(r, - "Failed to rename temporary file '%s' to target file '%s': %m", - editfile->temp, - editfile->path); - editfile->temp = mfree(editfile->temp); log_info("Successfully installed edited file '%s'.", editfile->path); } From 119cba78353e5d8337bfde0a66a0dcd707072e8b Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Fri, 5 Jul 2024 21:34:07 +0200 Subject: [PATCH 4/4] networkctl: support edit --stdin --- man/networkctl.xml | 20 ++++++++++++++++++++ src/network/networkctl-config-file.c | 13 ++++++++++--- src/network/networkctl.c | 8 ++++++++ src/network/networkctl.h | 1 + src/systemctl/systemctl-edit.c | 2 +- src/systemctl/systemctl.c | 2 +- test/units/TEST-74-AUX-UTILS.networkctl.sh | 7 ++++--- 7 files changed, 45 insertions(+), 8 deletions(-) diff --git a/man/networkctl.xml b/man/networkctl.xml index 5e2126ff218..e47cf5895cf 100644 --- a/man/networkctl.xml +++ b/man/networkctl.xml @@ -448,6 +448,9 @@ s - Service VLAN, m - Two-port MAC Relay (TPMR) systemd.link5 for more information. + If is specified, the new content will be read from standard input. + In this mode, the old content of the file is discarded. + @@ -608,6 +611,23 @@ s - Service VLAN, m - Two-port MAC Relay (TPMR) + + + + + When used with edit, the contents of the file will be read from standard + input and the editor will not be launched. In this mode, the old contents of the file are + automatically replaced. This is useful to "edit" configuration from scripts, especially so that + drop-in directories are created and populated in one go. + + Multiple drop-ins may be "edited" in this mode with , and + the same contents will be written to all of them. Otherwise exactly one main configuration file + is expected. + + + + + diff --git a/src/network/networkctl-config-file.c b/src/network/networkctl-config-file.c index 216e9d49543..1aa03619b72 100644 --- a/src/network/networkctl-config-file.c +++ b/src/network/networkctl-config-file.c @@ -396,23 +396,30 @@ static int reload_daemons(ReloadFlags flags) { } int verb_edit(int argc, char *argv[], void *userdata) { + char **args = ASSERT_PTR(strv_skip(argv, 1)); _cleanup_(edit_file_context_done) EditFileContext context = { .marker_start = DROPIN_MARKER_START, .marker_end = DROPIN_MARKER_END, .remove_parent = !!arg_drop_in, + .stdin = arg_stdin, }; _cleanup_(sd_netlink_unrefp) sd_netlink *rtnl = NULL; ReloadFlags reload = 0; int r; - if (!on_tty()) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot edit network config files if not on a tty."); + if (!on_tty() && !arg_stdin) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot edit network config files interactively if not on a tty."); + + /* Duplicating main configs makes no sense. This also mimics the behavior of systemctl. */ + if (arg_stdin && !arg_drop_in && strv_length(args) != 1) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "When 'edit --stdin' without '--drop-in=', exactly one config file for editing must be specified."); r = mac_selinux_init(); if (r < 0) return r; - STRV_FOREACH(name, strv_skip(argv, 1)) { + STRV_FOREACH(name, args) { _cleanup_strv_free_ char **dropins = NULL; _cleanup_free_ char *path = NULL; const char *link_config; diff --git a/src/network/networkctl.c b/src/network/networkctl.c index b51c7aa7e59..6c96a84c46b 100644 --- a/src/network/networkctl.c +++ b/src/network/networkctl.c @@ -91,6 +91,7 @@ bool arg_all = false; bool arg_stats = false; bool arg_full = false; bool arg_runtime = false; +bool arg_stdin = false; unsigned arg_lines = 10; char *arg_drop_in = NULL; sd_json_format_flags_t arg_json_format_flags = SD_JSON_FORMAT_OFF; @@ -3025,6 +3026,7 @@ static int help(void) { " after editing network config\n" " --drop-in=NAME Edit specified drop-in instead of main config file\n" " --runtime Edit runtime config files\n" + " --stdin Read new contents of edited file from stdin\n" "\nSee the %s for details.\n", program_invocation_short_name, ansi_highlight(), @@ -3043,6 +3045,7 @@ static int parse_argv(int argc, char *argv[]) { ARG_NO_RELOAD, ARG_DROP_IN, ARG_RUNTIME, + ARG_STDIN, }; static const struct option options[] = { @@ -3058,6 +3061,7 @@ static int parse_argv(int argc, char *argv[]) { { "no-reload", no_argument, NULL, ARG_NO_RELOAD }, { "drop-in", required_argument, NULL, ARG_DROP_IN }, { "runtime", no_argument, NULL, ARG_RUNTIME }, + { "stdin", no_argument, NULL, ARG_STDIN }, {} }; @@ -3092,6 +3096,10 @@ static int parse_argv(int argc, char *argv[]) { arg_runtime = true; break; + case ARG_STDIN: + arg_stdin = true; + break; + case ARG_DROP_IN: if (isempty(optarg)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Empty drop-in file name."); diff --git a/src/network/networkctl.h b/src/network/networkctl.h index 1765a8f83df..d44ee8173e0 100644 --- a/src/network/networkctl.h +++ b/src/network/networkctl.h @@ -15,6 +15,7 @@ extern bool arg_all; extern bool arg_stats; extern bool arg_full; extern bool arg_runtime; +extern bool arg_stdin; extern unsigned arg_lines; extern char *arg_drop_in; extern sd_json_format_flags_t arg_json_format_flags; diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index 15398f83646..b7bd3851fd0 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -323,7 +323,7 @@ int verb_edit(int argc, char *argv[], void *userdata) { int r; if (!on_tty() && !arg_stdin) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot edit units if not on a tty."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot edit units interactively if not on a tty."); if (arg_transport != BUS_TRANSPORT_LOCAL) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot edit units remotely."); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 1e36455cf21..912c1d83017 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -347,7 +347,7 @@ static int systemctl_help(void) { " --drop-in=NAME Edit unit files using the specified drop-in file name\n" " --when=TIME Schedule halt/power-off/reboot/kexec action after\n" " a certain timestamp\n" - " --stdin Read contents of edited file from stdin\n" + " --stdin Read new contents of edited file from stdin\n" "\nSee the %2$s for details.\n", program_invocation_short_name, link, diff --git a/test/units/TEST-74-AUX-UTILS.networkctl.sh b/test/units/TEST-74-AUX-UTILS.networkctl.sh index 3e333a2eabe..8c62de9155a 100755 --- a/test/units/TEST-74-AUX-UTILS.networkctl.sh +++ b/test/units/TEST-74-AUX-UTILS.networkctl.sh @@ -11,7 +11,7 @@ at_exit() { systemctl stop systemd-networkd if [[ -v NETWORK_NAME && -v NETDEV_NAME && -v LINK_NAME ]]; then - rm -fvr {/usr/lib,/etc,/run}/systemd/network/"$NETWORK_NAME" "/usr/lib/systemd/network/$NETDEV_NAME" \ + rm -fvr {/usr/lib,/etc,/run}/systemd/network/"$NETWORK_NAME" "/run/lib/systemd/network/$NETDEV_NAME" \ {/usr/lib,/etc}/systemd/network/"$LINK_NAME" "/etc/systemd/network/${NETWORK_NAME}.d" \ "new" "+4" fi @@ -75,13 +75,14 @@ cmp "+4" "/etc/systemd/network/${NETWORK_NAME}.d/test.conf" networkctl cat "$NETWORK_NAME" | grep '^# ' | cmp - <(printf '%s\n' "# /etc/systemd/network/$NETWORK_NAME" "# /etc/systemd/network/${NETWORK_NAME}.d/test.conf") -cat >"/usr/lib/systemd/network/$NETDEV_NAME" <"/usr/lib/systemd/network/$LINK_NAME" <