[libvirt] [PATCH 0/2] virsh: honor VISUAL
On IRC yesterday, the comment came up that 'virsh edit' is over-protective, because it prevented me from editing in-terminal using my favorite editor, even after I worked around the fact that sudo sanitizes EDITOR. Besides, historically, VISUAL is used in situations where opening a new window is okay, while EDITOR is used when editing should reuse the current terminal; so we should honor that convention.
* tools/virsh.pod: (DESCRIPTION): Improve grammar. Mention other drivers. (ENVIRONMENT): Document EDITOR. (COPYRIGHT): Bump. --- tools/virsh.pod | 16 ++++++++++++++-- 1 files changed, 14 insertions(+), 2 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index 8f6df19..302de18 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -10,7 +10,15 @@ virsh <subcommand> [args] The B<virsh> program is the main interface for managing virsh guest domains. The program can be used to create, pause, and shutdown -domains. It can also be used to list current domains. Libvirt is a C toolkit to interact with the virtualization capabilities of recent versions of Linux (and other OSes). It is free software available under the GNU Lesser General Public License. Virtualization of the Linux Operating System means the ability to run multiple instances of Operating Systems concurrently on a single hardware system where the basic resources are driven by a Linux instance. The library aim at providing long term stable C API initially for the Xen paravirtualization but should be able to integrate other virtualization mechanisms, it currently also support QEmu and KVM. +domains. It can also be used to list current domains. Libvirt is a C +toolkit to interact with the virtualization capabilities of recent +versions of Linux (and other OSes). It is free software available +under the GNU Lesser General Public License. Virtualization of the +Linux Operating System means the ability to run multiple instances of +Operating Systems concurrently on a single hardware system where the +basic resources are driven by a Linux instance. The library aims at +providing a long term stable C API. It currently supports Xen, QEmu, +KVM, LXC, OpenVZ, VirtualBox, OpenNebula, and VMware ESX. The basic structure of most virsh usage is: @@ -631,6 +639,10 @@ of C<virsh> The hypervisor to connect to by default. Set this to a URI, in the same format as accepted by the B<connect> option. +=item EDITOR + +The editor to use by the B<edit> and B<net-edit> options. + =item LIBVIRT_DEBUG=LEVEL Turn on verbose debugging of all libvirt API calls. Valid levels are @@ -676,7 +688,7 @@ Alternatively report bugs to your software distributor / vendor. =head1 COPYRIGHT -Copyright (C) 2005, 2007-2009 Red Hat, Inc. +Copyright (C) 2005, 2007-2010 Red Hat, Inc. =head1 LICENSE -- 1.6.6.1
2010/3/12 Eric Blake <eblake@redhat.com>:
* tools/virsh.pod: (DESCRIPTION): Improve grammar. Mention other drivers. (ENVIRONMENT): Document EDITOR. (COPYRIGHT): Bump. --- tools/virsh.pod | 16 ++++++++++++++-- 1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.pod b/tools/virsh.pod index 8f6df19..302de18 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -10,7 +10,15 @@ virsh <subcommand> [args]
The B<virsh> program is the main interface for managing virsh guest domains. The program can be used to create, pause, and shutdown -domains. It can also be used to list current domains. Libvirt is a C toolkit to interact with the virtualization capabilities of recent versions of Linux (and other OSes). It is free software available under the GNU Lesser General Public License. Virtualization of the Linux Operating System means the ability to run multiple instances of Operating Systems concurrently on a single hardware system where the basic resources are driven by a Linux instance. The library aim at providing long term stable C API initially for the Xen paravirtualization but should be able to integrate other virtualization mechanisms, it currently also support QEmu and KVM. +domains. It can also be used to list current domains. Libvirt is a C +toolkit to interact with the virtualization capabilities of recent +versions of Linux (and other OSes). It is free software available +under the GNU Lesser General Public License. Virtualization of the +Linux Operating System means the ability to run multiple instances of +Operating Systems concurrently on a single hardware system where the +basic resources are driven by a Linux instance. The library aims at +providing a long term stable C API. It currently supports Xen, QEmu, +KVM, LXC, OpenVZ, VirtualBox, OpenNebula, and VMware ESX.
The basic structure of most virsh usage is:
@@ -631,6 +639,10 @@ of C<virsh> The hypervisor to connect to by default. Set this to a URI, in the same format as accepted by the B<connect> option.
+=item EDITOR + +The editor to use by the B<edit> and B<net-edit> options. +
There is also a pool-edit and iface-edit command. ACK. Matthias
On 03/13/2010 07:35 AM, Matthias Bolte wrote:
+=item EDITOR + +The editor to use by the B<edit> and B<net-edit> options. +
There is also a pool-edit and iface-edit command.
Good point; I missed them because they did not have the same level of documentation to begin with. Should I respin the patch? I'm on the road, and won't be able to get to it before Wednesday, if someone wants to pick up where I left off before then... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Common Unix practice is to prefer VISUAL over EDITOR, particularly if the editor of choice spawns a new window. Thus, it is also common to see settings like EDITOR='emacs -nw', with the expectation that the shell will parse this as an argument to 'emacs' and not try to invoke a file containing a space. If a user puts junk in EDITOR, they deserve what they get (much more than virsh will misbehave); furthermore, sudo scrubs EDITOR by default. So the blind use of metacharacters in EDITOR should not be considered too much of a security issue. * tools/virsh.c (editFile): Prefer VISUAL over EDITOR. Don't reject shell metacharacters in EDITOR. * tools/virsh.pod (edit, net-edit, ENVIRONMENT): Document VISUAL. --- tools/virsh.c | 19 ++++++++----------- tools/virsh.pod | 15 ++++++++++----- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index aa85ee6..9f3b197 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7381,20 +7381,17 @@ editFile (vshControl *ctl, const char *filename) char *command; int command_ret; - editor = getenv ("EDITOR"); + editor = getenv ("VISUAL"); + if (!editor) editor = getenv ("EDITOR"); if (!editor) editor = "vi"; /* could be cruel & default to ed(1) here */ - /* Check the editor doesn't contain shell meta-characters, and if - * it does, refuse to run. + /* 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). */ - if (strspn (editor, ACCEPTED_CHARS) != strlen (editor)) { - vshError(ctl, - _("%s: $EDITOR environment variable contains shell meta or " - "other unacceptable characters"), - editor); - return -1; - } - /* Same for the filename. */ if (strspn (filename, ACCEPTED_CHARS) != strlen (filename)) { vshError(ctl, _("%s: temporary filename contains shell meta or other " diff --git a/tools/virsh.pod b/tools/virsh.pod index 302de18..3707916 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -332,8 +332,8 @@ This is equivalent to: virsh define domain.xml except that it does some error checking. -The editor used can be supplied by the C<$EDITOR> environment -variable, or if that is not defined defaults to C<vi>. +The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment +variables, and defaults to C<vi>. =item B<migrate> optional I<--live> I<--suspend> I<domain-id> I<desturi> I<migrateuri> @@ -557,8 +557,8 @@ This is equivalent to: virsh define network.xml except that it does some error checking. -The editor used can be supplied by the C<$EDITOR> environment -variable, or if that is not defined defaults to C<vi>. +The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment +variables, and defaults to C<vi>. =item B<net-list> optional I<--inactive> or I<--all> @@ -639,10 +639,15 @@ of C<virsh> The hypervisor to connect to by default. Set this to a URI, in the same format as accepted by the B<connect> option. -=item EDITOR +=item VISUAL The editor to use by the B<edit> and B<net-edit> options. +=item EDITOR + +The editor to use by the B<edit> and B<net-edit> options, if C<VISUAL> +is not set. + =item LIBVIRT_DEBUG=LEVEL Turn on verbose debugging of all libvirt API calls. Valid levels are -- 1.6.6.1
2010/3/12 Eric Blake <eblake@redhat.com>:
Common Unix practice is to prefer VISUAL over EDITOR, particularly if the editor of choice spawns a new window. Thus, it is also common to see settings like EDITOR='emacs -nw', with the expectation that the shell will parse this as an argument to 'emacs' and not try to invoke a file containing a space.
If a user puts junk in EDITOR, they deserve what they get (much more than virsh will misbehave); furthermore, sudo scrubs EDITOR by default. So the blind use of metacharacters in EDITOR should not be considered too much of a security issue.
* tools/virsh.c (editFile): Prefer VISUAL over EDITOR. Don't reject shell metacharacters in EDITOR. * tools/virsh.pod (edit, net-edit, ENVIRONMENT): Document VISUAL. ---
@@ -639,10 +639,15 @@ of C<virsh> The hypervisor to connect to by default. Set this to a URI, in the same format as accepted by the B<connect> option.
-=item EDITOR +=item VISUAL
The editor to use by the B<edit> and B<net-edit> options.
+=item EDITOR + +The editor to use by the B<edit> and B<net-edit> options, if C<VISUAL> +is not set. +
There is also a pool-edit and iface-edit command. ACK. Matthias
On 03/13/2010 07:35 AM, Matthias Bolte wrote:
2010/3/12 Eric Blake <eblake@redhat.com>:
Common Unix practice is to prefer VISUAL over EDITOR, particularly if the editor of choice spawns a new window. Thus, it is also common to see settings like EDITOR='emacs -nw', with the expectation that the shell will parse this as an argument to 'emacs' and not try to invoke a file containing a space.
+=item EDITOR + +The editor to use by the B<edit> and B<net-edit> options, if C<VISUAL> +is not set. +
There is also a pool-edit and iface-edit command.
It turns out these are not documented at all in virsh.pod (that is, it is a separate patch to bring all of virsh.pod up-to-date to match current implementation, which I hope to submit soon).
ACK.
Pushed as-is, in light of bug 487738 that promised this by the next release (the doc cleanups shouldn't hold up the ACK'd functionality fix). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Mar 12, 2010 at 09:50:01AM -0700, Eric Blake wrote:
Common Unix practice is to prefer VISUAL over EDITOR, particularly if the editor of choice spawns a new window. Thus, it is also common to see settings like EDITOR='emacs -nw', with the expectation that the shell will parse this as an argument to 'emacs' and not try to invoke a file containing a space.
If a user puts junk in EDITOR, they deserve what they get (much more than virsh will misbehave); furthermore, sudo scrubs EDITOR by default. So the blind use of metacharacters in EDITOR should not be considered too much of a security issue.
* tools/virsh.c (editFile): Prefer VISUAL over EDITOR. Don't reject shell metacharacters in EDITOR. * tools/virsh.pod (edit, net-edit, ENVIRONMENT): Document VISUAL. --- tools/virsh.c | 19 ++++++++----------- tools/virsh.pod | 15 ++++++++++----- 2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index aa85ee6..9f3b197 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7381,20 +7381,17 @@ editFile (vshControl *ctl, const char *filename) char *command; int command_ret;
- editor = getenv ("EDITOR"); + editor = getenv ("VISUAL"); + if (!editor) editor = getenv ("EDITOR"); if (!editor) editor = "vi"; /* could be cruel & default to ed(1) here */
- /* Check the editor doesn't contain shell meta-characters, and if - * it does, refuse to run. + /* 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). */ - if (strspn (editor, ACCEPTED_CHARS) != strlen (editor)) { - vshError(ctl, - _("%s: $EDITOR environment variable contains shell meta or " - "other unacceptable characters"), - editor); - return -1; - } - /* Same for the filename. */ if (strspn (filename, ACCEPTED_CHARS) != strlen (filename)) { vshError(ctl, _("%s: temporary filename contains shell meta or other " diff --git a/tools/virsh.pod b/tools/virsh.pod index 302de18..3707916 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -332,8 +332,8 @@ This is equivalent to: virsh define domain.xml except that it does some error checking.
-The editor used can be supplied by the C<$EDITOR> environment -variable, or if that is not defined defaults to C<vi>. +The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment +variables, and defaults to C<vi>.
=item B<migrate> optional I<--live> I<--suspend> I<domain-id> I<desturi> I<migrateuri>
@@ -557,8 +557,8 @@ This is equivalent to: virsh define network.xml except that it does some error checking.
-The editor used can be supplied by the C<$EDITOR> environment -variable, or if that is not defined defaults to C<vi>. +The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment +variables, and defaults to C<vi>.
=item B<net-list> optional I<--inactive> or I<--all>
@@ -639,10 +639,15 @@ of C<virsh> The hypervisor to connect to by default. Set this to a URI, in the same format as accepted by the B<connect> option.
-=item EDITOR +=item VISUAL
The editor to use by the B<edit> and B<net-edit> options.
+=item EDITOR + +The editor to use by the B<edit> and B<net-edit> options, if C<VISUAL> +is not set. + =item LIBVIRT_DEBUG=LEVEL
Turn on verbose debugging of all libvirt API calls. Valid levels are --
ACK This fixes this bug too https://bugzilla.redhat.com/show_bug.cgi?id=487738 Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Matthias Bolte