2011/1/28 Eric Blake <ebl...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
