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

[Ronny Chevalier]

2014-10-21 11:01 GMT+02:00 Lennart Poettering <lennart at poettering.net>:
> On Tue, 21.10.14 03:01, Ronny Chevalier (chevalier.ronny at gmail.com) wrote:
>
>> >> +static int get_editors(char ***editors) {
>> >> +        char **tmp_editors = strv_new("nano", "vim", "vi", NULL);
>> >
>> > Please avoid calling functions and declaring variables in one line.
>> >
>> > Also, there's an OOM check missing for this line.
>> >
>> >> +        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;
>> >> +}
>> >
>> > Hmm, I don't like this bit. Instead of figuring out the editor in one
>> > step, and then executing it, I'd really prefer if we'd try to execute
>> > the editors one-by-one until we find one that works. When we invoke
>> > the pager we follow a similar logic.
>>
>> Actually that's what I do. I fill a strv of editors and try to execute
>> each one of them, until one works. But we fail if the error is
>> different than "No such file or directory".
>>
>> This function (get_editors) is used in run_editor, where I do a
>> STRV_FOREACH in the child to try to execute every editor, with the one
>> comming from SYSTEMD_EDITOR or EDITOR or VISUAL, first.
>
> Oh, well, indeed. Hmm, so I figure I'd just prefer if we wouldn't have
> to allocate memory for this. or in other words, I'd like to see an alg
> like this:
>
> if (fork() == 0) {
>
>         ...
>
>         e = getenv("SYSTEMD_EDITOR");
>         if (!e)
>                 e = getenv("EDITOR");
>         if (!e)
>                 e = getenv("VISUAL");
>         if (e)
>                 execlp(e, ...);
>
>         execlp("nano", ...);
>         execlp("vim", ...);
>         execlp("vi", ...);
>
>         ...
> }
>
> That way we wouldn't have to allocate an strv, and just open code the
> search logic. This would then mirror nicely how the pager logic works...
I agree but I can't (or I don't know how to do this), because we need
to give the paths of the units to execvp, and since the first argument
must be the name of the editor, I need to add it to the strv. The
original strv is a strv of units path, so I copy it and prepend the
name of the editor.

We can do this for the pager because there is no arguments to give,
here we have a list of paths.

>
>> Or maybe, you meant that if the one coming from SYSTEMD_EDITOR failed,
>> we should try to execute EDITOR, then VISUAL, too? Because I think we
>> should do that, but with the current code we don't.
>
> I think it's OK checking only the first of SYSTEMD_EDITOR, EDITOR,
> VISUAL that exists, and then proceed with nano/vim/vi
Ok

>
>> >> +        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;
>> >> +        }
>> >
>> > Hmm, why not support this? And imply --runtime or so?
>>
>> You are right, I forgot to ask a question about this one, sorry.
>>
>> If there is a unit in /etc/systemd/system/ and someone wants to edit
>> this unit temporarily it will run systemctl --runtime myunit. The
>> problem is the unit will be copied in the /run/ directory but this
>> directory do not have precedence over /etc/systemd/, so when this unit
>> will be restarted, it won't take into account the one in /run/, right?
>> (My understanding of this come from src/shared/path-lookup.c)
>
> Indeed. We should probably print an error in this case and refuse
> operation. However we should support adding a --runtime .d/ snippet if
> a unit file is in /etc, because that will actually work fine.
I think you meant /usr/.

Ok
>
>> Furthermore, if I recall correctly the FragmentPath of a runtime
>> unit is empty?
>
> No, it should be set properly like in all other cases really.
>
>> So, if I'm wrong and systemctl understands that when we do restart
>> with --runtime it means to execute the one from /run/, well I can
>> certainly add support for this.
>>
>> If I'm a right, I do not see the point because it will not be easy to
>> understand for a user that it can temporarily edit a unit if it's in
>> /usr/lib/systemd/ but not in /etc/.
>
> Yeah it is suprising but I think we could explain it nicely in an
> error.
>
> I'd merge your patch with or without this btw, I think it would just
> be nice to support --runtime to the level we can support it, but it's
> certainly just the cherry on top.
I don't think it will take too much time, so I will resend a patch
with support for this (and corrections for the rest of the comments)

>
> Lennart
>
> --
> Lennart Poettering, Red Hat