[systemd-devel] [PATCH v3] systemctl: add edit verb

Zbigniew Jędrzejewski-Szmek zbyszek at in.waw.pl
Tue Oct 21 18:02:14 PDT 2014


On Wed, Oct 22, 2014 at 02:42:13AM +0200, Ronny Chevalier wrote:
> 2014-10-22 2:13 GMT+02:00 Ronny Chevalier <chevalier.ronny at gmail.com>:
> > 2014-10-22 1:48 GMT+02:00 Zbigniew Jędrzejewski-Szmek <zbyszek at in.waw.pl>:
> >> 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.
> > You are right, I will rework it this way.
> >
> >>
> >> I'm not sure abou the name 'amendments.conf'. Wouldn't 'local.conf'
> >> be more idiomatic, and also easier to type?
> > Ok
> >
> >>
> >>> 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="..." />.
> > Ok I will look into this.
> >
> >>
> >>> -        <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.
> > You are right, it's better this way.
> >
> >>
> >>> +
> >>> +            <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.
> > I use LookupPaths, created with lookup_paths_init, which already
> > handle arg_root.
> >
But this function below and the next one do not honour arg_root, no?

> >>> +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.
> Oh and for this one, I found wait_for_terminate_and_warn which handles
> Lennart comment and yours.
> 
> >>
> >>> +}
> >>> +
> >>> +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.
> > Yeah it seems better. While thinking about this, would it be better if
> > we fail in this case whether than no-op without saying nothing ? I
> > can't see why someone would use this on non-tty ?
You're right. Especially if you implement the temporary-file-first approach,
it doesn't make much sense to do anything if not on tty.

> >
> >>
> >>> +
> >>> +        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.
> > Actually this is aligned, just the two before this one are not, do you
> > want me to align them ?
Yeah, it's nicer if everything is aligned.


> >>
> >>>                  {}
> >>>          }, *verb = verbs;
> >> Zbyszek
> >
> > Thanks for the review
My pleasure.

Zbyszek


More information about the systemd-devel mailing list