[systemd-devel] [PATCH v3] systemctl: add edit verb
Ronny Chevalier
chevalier.ronny at gmail.com
Tue Oct 21 17:13:43 PDT 2014
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.
>
>> +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.
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 ?
>
>> +
>> + 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 ?
>
>> {}
>> }, *verb = verbs;
> Zbyszek
Thanks for the review
More information about the systemd-devel
mailing list