[libvirt] [PATCH] virsh: Fix improper freecell --cellno command behavior

[Eric Blake]

On 02/15/2011 01:09 AM, Michal Privoznik wrote:
> Non-digit characters were accepted in --cellno resulting in
> unwanted behavior: 'virsh freecell --cellno foo' wasn't
> rejected.
> ---
> This fix addresses bug 639587
> 
>  tools/virsh.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index c2d165d..765566d 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -2283,7 +2283,7 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd)
>  {
>      int func_ret = FALSE;
>      int ret;
> -    int cell, cell_given;
> +    int cell, cell_given, cellStr_given;
>      unsigned long long memory;
>      unsigned long long *nodes = NULL;
>      int all_given;
> @@ -2295,6 +2295,13 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd)
>  
>      cell = vshCommandOptInt(cmd, "cellno", &cell_given);
>      all_given = vshCommandOptBool(cmd, "all");
> +    vshCommandOptString(cmd, "cellno", &cellStr_given);

Hmm.  Why are we parsing "cellno" twice, once with OptInt, and again
with OptString?

We REALLY need to overhaul virsh.c to make the parsers return tri-state
information (no option present, option present and parse valid, option
present but parse invalid), and update all callers accordingly.

In fact, there's already a thread on this exact same topic:
https://www.redhat.com/archives/libvir-list/2011-January/msg00132.html

with my thoughts on a better rewrite:
https://www.redhat.com/archives/libvir-list/2011-January/msg00145.html

> +
> +    if (cellStr_given && !cell_given) {
> +        vshError(ctl, "%s", _("cell number must not contain any "
> +                              "non-digit characters."));
> +        goto cleanup;
> +    }

However, your patch does have the advantage over Osier's first attempt,
in that it does not affect all other callers and does not give a
misleading message for --cellno=-1.

Unless anyone else has any strong opinions on getting this fixed prior
to 0.8.8, my inclination would be to postpone this issue until
post-release (after all, it only affects users that pass invalid option
arguments; it doesn't affect valid uses of the freecell command), and go
with the full-blown rewrite of the option argument parsing routines to
better handle tri-state detection and default values.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110215/7cad758e/attachment-0001.sig>