[libvirt] [PATCH 1/2] build: avoid close, system

[Matthias Bolte]

2011/1/28 Eric Blake <eblake at redhat.com>:
> * src/fdstream.c (virFDStreamOpenFile, virFDStreamCreateFile):
> Use VIR_FORCE_CLOSE instead of close.
> * tests/commandtest.c (mymain): Likewise.
> * tools/virsh.c (editFile): Use virCommand instead of system.
> ---
>  src/fdstream.c      |    6 ++--
>  tests/commandtest.c |   12 +++++++---
>  tools/virsh.c       |   59 +++++++++++++++++++++++---------------------------
>  3 files changed, 38 insertions(+), 39 deletions(-)


> diff --git a/tools/virsh.c b/tools/virsh.c
> index cd54174..cf482e3 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c

>     editor = getenv ("VISUAL");
> -    if (!editor) editor = getenv ("EDITOR");
> -    if (!editor) editor = "vi"; /* could be cruel & default to ed(1) here */
> +    if (!editor)
> +        editor = getenv ("EDITOR");
> +    if (!editor)
> +        editor = "vi"; /* could be cruel & default to ed(1) here */

When VISUAL and EDITOR isn't set we fallback to vi here, but ...

>     /* Check that filename doesn't contain shell meta-characters, and
>      * if it does, refuse to run.  Follow the Unix conventions for
>      * EDITOR: the user can intentionally specify command options, so
>      * we don't protect any shell metacharacters there.  Lots more
>      * than virsh will misbehave if EDITOR has bogus contents (which
> -     * is why sudo scrubs it by default).
> +     * is why sudo scrubs it by default).  Conversely, if the editor
> +     * is safe, we can run it directly rather than wasting a shell.
>      */
> -    if (strspn (filename, ACCEPTED_CHARS) != strlen (filename)) {
> -        vshError(ctl,
> -                 _("%s: temporary filename contains shell meta or other "
> -                   "unacceptable characters (is $TMPDIR wrong?)"),
> -                 filename);
> -        return -1;
> +    if (strspn (editor, ACCEPTED_CHARS) != strlen (editor)) {
> +        if (strspn (filename, ACCEPTED_CHARS) != strlen (filename)) {
> +            vshError(ctl,
> +                     _("%s: temporary filename contains shell meta or other "
> +                       "unacceptable characters (is $TMPDIR wrong?)"),
> +                     filename);
> +            return -1;
> +        }
> +        cmd = virCommandNewArgList("sh", "-c", NULL);
> +        virCommandAddArgFormat(cmd, "%s %s", editor, filename);
> +    } else {
> +        cmd = virCommandNewArgList("editor", filename, NULL);
>     }

... here you made it fallback to editor instead. Shouldn't this be
consistent and fallback to the same in both cases?

Anyway, that's minor and doesn't affect my ACK.

Matthias