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

[Lennart Poettering]

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...

> 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

> >> +        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.

> 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.

Lennart

-- 
Lennart Poettering, Red Hat