[systemd-devel] [PATCH v3] systemctl: add edit verb
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Tue Oct 21 16:48:31 PDT 2014
On Sat, Oct 18, 2014 at 06:30:02PM +0200, Ronny Chevalier wrote:
> It helps editing units by either creating a drop-in file, like
> /etc/systemd/system/my.service.d/amendments.conf, or by copying the
> original unit from /usr/lib/systemd/ to /etc/systemd/ if the --full
> option is specified. Then it invokes an editor to the related files
> and daemon-reload is invoked when the editor exited successfully.
Hm, this sequence doesn't sound right. A temporary file should be
created, edited, and then atomically put in place, iff the editor
exits successfully. I think we should follow in the footsteps of git
here... and abort if the editor exits with an error.
I'm not sure abou the name 'amendments.conf'. Wouldn't 'local.conf'
be more idiomatic, and also easier to type?
> See https://bugzilla.redhat.com/show_bug.cgi?id=906824
> ---
> TODO | 2 -
> man/journalctl.xml | 6 +-
> man/less-variables.xml | 40 +++--
> man/localectl.xml | 6 +-
> man/loginctl.xml | 6 +-
> man/machinectl.xml | 6 +-
> man/systemctl.xml | 49 +++++-
> man/systemd-analyze.xml | 6 +-
> man/timedatectl.xml | 6 +-
> src/systemctl/systemctl.c | 394 +++++++++++++++++++++++++++++++++++++++++++++-
> 10 files changed, 488 insertions(+), 33 deletions(-)
There's no need to mangle all the xml files. It is possible
to include specific parts of a file. See how standard-options.xml
incorporated whole, and sometimes just specific parts using
<xi:include href="standard-options.xml" xpointer="..." />.
> - <xi:include href="less-variables.xml" />
> + <refsect1>
> + <title>Environment</title>
> +
> + <xi:include href="less-variables.xml" />
> + </refsect1>
>
> + <varlistentry>
> + <term><command>edit <replaceable>NAME</replaceable>...</command></term>
> +
> + <listitem>
> + <para>Edit one or more unit files, as specified on the command
> + line.</para>
This wording is misleading, because the unit file actually will not be *edited*,
but extended in the normal case where --full is not used.
I'm missing an explanatory sentence here, something like "An editor will be launched
to edit a drop-in snippet (or a whole replacement file if --full is used), to extend
or override the specified unit." Then the next paragraph about --system/--user/--global
will be more natural.
> +
> + <para>Depending on whether <option>--system</option> (the default),
> + <option>--user</option>, or <option>--global</option> is specified,
> + this create a drop-in file for each units either for the system,
> + for the calling user or for all futures logins of all users. Then
> + the editor is invoked on them (see section "Environment" below).</para>
> +
> + <para>If <option>--full</option> is specified, this will copy the original
> + units instead of creating drop-in files.</para>
> +
> + <para>After the units have been edited, the systemd configuration is
> + reloaded (in a way that is equivalent to <command>daemon-reload</command>),
> + but it does not restart or reload the units.</para>
> +
> + <para>Note that this command cannot be used with <option>--runtime</option> or
> + to remotely edit units.</para>
> + </listitem>
> + </varlistentry>
> </variablelist>
> </refsect2>
>
> + <variablelist class='environment-variables'>
> + <varlistentry>
> + <term><varname>$SYSTEMD_EDITOR</varname></term>
> +
> + <listitem><para>Editor to use when editing units; overrides
> + <varname>$EDITOR</varname> and <varname>$VISUAL</varname>. If neither
> + <varname>$SYSTEMD_EDITOR</varname> nor <varname>$EDITOR</varname> nor
> + <varname>$VISUAL</varname> are present or if it is set to an empty
> + string or if their execution failed, systemctl will try to execute well
> + known editors in this order:
> + <citerefentry><refentrytitle>nano</refentrytitle><manvolnum>1</manvolnum></citerefentry>,
> + <citerefentry><refentrytitle>vim</refentrytitle><manvolnum>1</manvolnum></citerefentry>,
> + <citerefentry><refentrytitle>vi</refentrytitle><manvolnum>1</manvolnum></citerefentry>.
> + </para></listitem>
> + </varlistentry>
> + </variablelist>
> + <xi:include href="less-variables.xml" />
> + </refsect1>
>
> <refsect1>
> <title>See Also</title>
> @@ -1572,7 +1617,7 @@ kobject-uevent 1 systemd-udevd-kernel.socket systemd-udevd.service
> <citerefentry><refentrytitle>systemd.resource-management</refentrytitle><manvolnum>5</manvolnum></citerefentry>,
> <citerefentry><refentrytitle>systemd.special</refentrytitle><manvolnum>7</manvolnum></citerefentry>,
> <citerefentry project='man-pages'><refentrytitle>wall</refentrytitle><manvolnum>1</manvolnum></citerefentry>,
> - <citerefentry><refentrytitle>systemd.preset</refentrytitle><manvolnum>5</manvolnum></citerefentry>
> + <citerefentry><refentrytitle>systemd.preset</refentrytitle><manvolnum>5</manvolnum></citerefentry>,
> <citerefentry><refentrytitle>glob</refentrytitle><manvolnum>7</manvolnum></citerefentry>
> </para>
> </refsect1>
> diff --git a/man/systemd-analyze.xml b/man/systemd-analyze.xml
> index 073e807..0dd21a5 100644
> --- a/man/systemd-analyze.xml
> +++ b/man/systemd-analyze.xml
> @@ -383,7 +383,11 @@ Service b at 0.service not loaded, b.socket cannot be started.
> </example>
> </refsect1>
>
> - <xi:include href="less-variables.xml" />
> + <refsect1>
> + <title>Environment</title>
> +
> + <xi:include href="less-variables.xml" />
> + </refsect1>
>
> <refsect1>
> <title>See Also</title>
> diff --git a/man/timedatectl.xml b/man/timedatectl.xml
> index f3edb8d..849cc06 100644
> --- a/man/timedatectl.xml
> +++ b/man/timedatectl.xml
> @@ -197,7 +197,11 @@
> code otherwise.</para>
> </refsect1>
>
> - <xi:include href="less-variables.xml" />
> + <refsect1>
> + <title>Environment</title>
> +
> + <xi:include href="less-variables.xml" />
> + </refsect1>
>
> <refsect1>
> <title>Examples</title>
> diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
> index 28eaa6a..619f7e0 100644
> --- a/src/systemctl/systemctl.c
> +++ b/src/systemctl/systemctl.c
> @@ -72,6 +72,8 @@
> #include "bus-message.h"
> #include "bus-error.h"
> #include "bus-errors.h"
> +#include "copy.h"
> +#include "mkdir.h"
>
> static char **arg_types = NULL;
> static char **arg_states = NULL;
> @@ -5642,6 +5644,393 @@ static int is_system_running(sd_bus *bus, char **args) {
> return streq(state, "running") ? EXIT_SUCCESS : EXIT_FAILURE;
> }
>
> +static int unit_file_find_path(LookupPaths *lp, const char *unit_name, char **unit_path) {
> + char **p;
> +
> + assert(lp);
> + assert(unit_name);
> + assert(unit_path);
> +
> + STRV_FOREACH(p, lp->unit_path) {
> + char *path;
> +
> + path = strjoin(*p, "/", unit_name, NULL);
> + if (!path)
> + return log_oom();
> +
> + if (access(path, F_OK) == 0) {
> + *unit_path = path;
> + return 1;
> + }
> +
> + free(path);
> + }
> +
> + return 0;
> +}
> +
I'm pretty sure we should support root_dir here. After all, we
support it for more of systemctl commands. It'd be especially convenient
in this case, since it's easy to mess up the paths for a chroot.
> +static int unit_file_drop_in(const char *unit_name, const char *config_home, char **new_path) {
> + char *tmp_path;
> + int r;
> +
> + assert(unit_name);
> + assert(new_path);
> +
> + switch (arg_scope) {
> + case UNIT_FILE_SYSTEM:
> + tmp_path = strjoin(SYSTEM_CONFIG_UNIT_PATH, "/", unit_name, ".d/amendments.conf", NULL);
> + break;
> + case UNIT_FILE_GLOBAL:
> + tmp_path = strjoin(USER_CONFIG_UNIT_PATH, "/", unit_name, ".d/amendments.conf", NULL);
> + break;
> + case UNIT_FILE_USER:
> + assert(config_home);
> + tmp_path = strjoin(config_home, "/", unit_name, ".d/amendments.conf", NULL);
> + break;
> + default:
> + assert_not_reached("Invalid scope");
> + }
> + if (!tmp_path)
> + return log_oom();
> +
> + r = mkdir_parents(tmp_path, 0755);
> + if (r < 0) {
> + log_error("Failed to create directories for %s: %s", tmp_path, strerror(-r));
> + free(tmp_path);
> + return r;
> + }
> +
> + *new_path = tmp_path;
> +
> + return 0;
> +}
> +
> +static int unit_file_copy_if_needed(const char *unit_name, const char *fragment_path, char **new_path) {
> + char *tmp_path;
> + int r;
> +
> + assert(fragment_path);
> + assert(unit_name);
> + assert(new_path);
> +
> + /* If it's a unit for the --user scope there is no need to copy it, it's already in the right directory.
> + * Same if this is --system/--global scope and the file is in {SYSTEM,USER}_CONFIG_UNIT_PATH
> + */
> + if (arg_scope == UNIT_FILE_USER
> + || startswith(fragment_path, SYSTEM_CONFIG_UNIT_PATH)
> + || startswith(fragment_path, USER_CONFIG_UNIT_PATH)) {
> + *new_path = strdup(fragment_path);
> + if (!*new_path)
> + return log_oom();
> + return 0;
> + }
> +
> + switch (arg_scope) {
> + case UNIT_FILE_SYSTEM:
> + tmp_path = strjoin(SYSTEM_CONFIG_UNIT_PATH, "/", unit_name, NULL);
> + break;
> + case UNIT_FILE_GLOBAL:
> + tmp_path = strjoin(USER_CONFIG_UNIT_PATH, "/", unit_name, NULL);
> + break;
> + default:
> + assert_not_reached("Invalid scope");
> + }
> + if (!tmp_path)
> + return log_oom();
> +
> + if (access(tmp_path, F_OK) == 0) {
> + char response;
> +
> + r = ask_char(&response, "yn", "%s already exists, are you sure to overwrite it with %s? [(y)es, (n)o] ", tmp_path, fragment_path);
> + if (r < 0) {
> + free(tmp_path);
> + return r;
> + }
> + if (response != 'y') {
> + log_warning("%s ignored", unit_name);
> + free(tmp_path);
> + return -1;
> + }
> + }
> +
> + r = mkdir_parents(tmp_path, 0755);
> + if (r < 0) {
> + log_error("Failed to create directories for %s: %s", tmp_path, strerror(-r));
> + free(tmp_path);
> + return r;
> + }
> +
> + r = copy_file(fragment_path, tmp_path, 0, 0644);
> + if (r < 0) {
> + log_error("Failed to copy %s to %s: %s", fragment_path, tmp_path, strerror(-r));
> + free(tmp_path);
> + return r;
> + }
> +
> + *new_path = tmp_path;
> +
> + return 0;
> +}
> +
> +static int get_editors(char ***editors) {
> + char **tmp_editors = strv_new("nano", "vim", "vi", NULL);
> + 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 editors
> + */
> + editor = getenv("SYSTEMD_EDITOR");
> + if (!editor)
> + editor = getenv("EDITOR");
> + if (!editor)
> + editor = getenv("VISUAL");
> +
> + if (editor) {
> + int r;
> +
> + editor = strdup(editor);
> + if (!editor)
> + return log_oom();
> +
> + r = strv_consume_prepend(&tmp_editors, editor);
> + if (r < 0)
> + return log_oom();
> + }
> +
> + *editors = tmp_editors;
> +
> + return 0;
> +}
> +
> +static int run_editor(char **paths) {
> + pid_t pid;
> + siginfo_t status;
> + int r;
> +
> + assert(paths);
> +
> + pid = fork();
> + if (pid < 0) {
> + log_error("Failed to fork: %m");
> + return -errno;
> + }
> +
> + if (pid == 0) {
> + _cleanup_strv_free_ char **editors = NULL;
> + char *editor;
> + char **p;
> +
> + r = get_editors(&editors);
> + if (r < 0) {
> + _exit(EXIT_FAILURE);
> + }
> +
> + STRV_FOREACH(p, editors) {
> + _cleanup_strv_free_ char **args = NULL;
> +
> + editor = strdup(*p);
> + if (!editor) {
> + log_oom();
> + _exit(EXIT_FAILURE);
> + }
> +
> + args = strv_copy(paths);
> + if (!args) {
> + log_oom();
> + _exit(EXIT_FAILURE);
> + }
> +
> + r = strv_consume_prepend(&args, editor);
> + if (r < 0) {
> + log_oom();
> + _exit(EXIT_FAILURE);
> + }
> +
> + execvp(editor, args);
> + /* We do not fail if the editor doesn't exist
> + * because we want to try each one of them before
> + * failing.
> + */
> + if (errno != ENOENT) {
> + log_error("Failed to execute %s: %m", editor);
> + _exit(EXIT_FAILURE);
> + }
> + }
> +
> + log_error("Cannot edit unit(s): No editor available. Please set either SYSTEMD_EDITOR or EDITOR or VISUAL environment variable");
> + _exit(EXIT_FAILURE);
> + }
> +
> + r = wait_for_terminate(pid, &status);
> + if (r < 0) {
> + log_error("Failed to wait for child: %s", strerror(-r));
> + return r;
> + }
> +
> + return WIFEXITED(status) ? WEXITSTATUS(status) : -1;
-EINVAL not -1.
> +}
> +
> +static int find_units_path(sd_bus *bus, char **names, char ***paths) {
> + _cleanup_free_ char *config_home = NULL;
> + char **name;
> + int r;
> +
> + assert(names);
> + assert(paths);
> +
> + if (arg_scope == UNIT_FILE_USER) {
> + r = user_config_home(&config_home);
> + if (r < 0)
> + return log_oom();
> +
> + if (r == 0) {
> + log_error("Cannot edit units for the user instance: home directory unknown");
> + return -1;
> + }
> + }
> +
> + if (!bus || avoid_bus()) {
> + _cleanup_lookup_paths_free_ LookupPaths lp = {};
> +
> + /* If there is no bus, we try to find the units by testing each available directory
> + * according to the scope.
> + */
> + r = lookup_paths_init(&lp,
> + arg_scope == UNIT_FILE_SYSTEM ? SYSTEMD_SYSTEM : SYSTEMD_USER,
> + arg_scope == UNIT_FILE_USER,
> + arg_root,
> + NULL, NULL, NULL);
> + if (r < 0) {
> + log_error("Cannot get lookup paths: %s", strerror(-r));
> + return r;
> + }
> +
> + STRV_FOREACH(name, names) {
> + _cleanup_free_ char *path = NULL;
> + char *new_path;
> +
> + r = unit_file_find_path(&lp, *name, &path);
> + if (r < 0)
> + return r;
> + if (r == 0) {
> + log_warning("%s ignored: not found", *name);
> + continue;
> + }
> +
> + if (arg_full)
> + r = unit_file_copy_if_needed(*name, path, &new_path);
> + else
> + r = unit_file_drop_in(*name, config_home, &new_path);
> +
> + if (r < 0)
> + continue;
> +
> + r = strv_push(paths, new_path);
> + if (r < 0)
> + return log_oom();
> + }
> + } else {
> + STRV_FOREACH(name, names) {
> + _cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
> + _cleanup_free_ char *fragment_path = NULL;
> + _cleanup_free_ char *unit = NULL;
> + char *new_path;
> +
> + unit = unit_dbus_path_from_name(*name);
> + if (!unit)
> + return log_oom();
> +
> + if (need_daemon_reload(bus, *name) > 0) {
> + log_warning("%s ignored: unit file changed on disk. Run 'systemctl%s daemon-reload'.",
> + *name, arg_scope == UNIT_FILE_SYSTEM ? "" : " --user");
> + continue;
> + }
> +
> + r = sd_bus_get_property_string(
> + bus,
> + "org.freedesktop.systemd1",
> + unit,
> + "org.freedesktop.systemd1.Unit",
> + "FragmentPath",
> + &error,
> + &fragment_path);
> + if (r < 0) {
> + log_warning("Failed to get FragmentPath: %s", bus_error_message(&error, r));
> + continue;
> + }
> +
> + if (isempty(fragment_path)) {
> + log_warning("%s ignored: not found", *name);
> + continue;
> + }
> +
> + if (arg_full)
> + r = unit_file_copy_if_needed(*name, fragment_path, &new_path);
> + else
> + r = unit_file_drop_in(*name, config_home, &new_path);
> + if (r < 0)
> + continue;
> +
> + r = strv_push(paths, new_path);
> + if (r < 0)
> + return log_oom();
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int edit(sd_bus *bus, char **args) {
> + _cleanup_strv_free_ char **names = NULL;
> + _cleanup_strv_free_ char **paths = NULL;
> + int r;
> +
> + assert(args);
> +
> + if (!on_tty())
> + return 0;
Shouldn't this check be later, just before run_editor? After all,
edit should fail the same for invalid use cases whether run
on tty or not.
> +
> + if (arg_transport != BUS_TRANSPORT_LOCAL) {
> + log_error("Cannot remotely edit units");
> + return -EINVAL;
> + }
> +
> + if (arg_runtime) {
> + log_error("Cannot edit runtime units");
> + return -EINVAL;
> + }
> +
> + r = expand_names(bus, args + 1, NULL, &names);
> + if (r < 0) {
> + log_error("Failed to expand names: %s", strerror(-r));
> + return r;
> + }
> +
> + if (!names) {
> + log_error("No unit name found by expanding names");
> + return -ENOENT;
> + }
> +
> + r = find_units_path(bus, names, &paths);
> + if (r < 0)
> + return r;
> +
> + if (strv_isempty(paths)) {
> + log_error("Cannot find any units to edit");
> + return -ENOENT;
> + }
> +
> + r = run_editor(paths);
> + if (r < 0)
> + return r;
> +
> + if (!arg_no_reload)
> + r = daemon_reload(bus, args);
> +
> + return r;
> +}
> +
> static void systemctl_help(void) {
>
> pager_open_if_enabled();
> @@ -5739,7 +6128,9 @@ static void systemctl_help(void) {
> " add-requires TARGET NAME... Add 'Requires' dependency for the target\n"
> " on specified one or more units\n"
> " get-default Get the name of the default target\n"
> - " set-default NAME Set the default target\n\n"
> + " set-default NAME Set the default target\n"
> + " edit NAME... Edit one or more unit files\n"
> + "\n"
> "Machine Commands:\n"
> " list-machines [PATTERN...] List local containers and host\n\n"
> "Job Commands:\n"
> @@ -6750,6 +7141,7 @@ static int systemctl_main(sd_bus *bus, int argc, char *argv[], int bus_error) {
> { "is-system-running", EQUAL, 1, is_system_running },
> { "add-wants", MORE, 3, add_dependency, NOBUS },
> { "add-requires", MORE, 3, add_dependency, NOBUS },
> + { "edit", MORE, 2, edit, NOBUS },
Please align the whole table ^ here.
> {}
> }, *verb = verbs;
Zbyszek
More information about the systemd-devel
mailing list