[systemd-devel] [PATCH v3] systemctl: add edit verb
Ronny Chevalier
chevalier.ronny at gmail.com
Tue Oct 21 17:42:13 PDT 2014
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.
>
>>
>>> +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 ?
>
>>
>>> +
>>> + 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