[libvirt] [PATCH] virsh: print error in case of cellno is invalid

[Eric Blake]

On 01/05/2011 07:03 AM, Osier Yang wrote:
> If invalid cellno is specified, command "freecell" will still
> print the amount of available memory of node. As a fix, print
> error instead.
> 
> * tools/virsh.c: "vshCommandOptInt", return -1 when value for
>   parameter is specified, but invalid, which means strtol was
>   failed, it won't affects other functions that use "vshCommandOptInt").
> ---
>  tools/virsh.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 55e2a68..31f2a54 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -2275,6 +2275,12 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd)
>          return FALSE;
> 
>      cell = vshCommandOptInt(cmd, "cellno", &cell_given);
> +
> +    if (cell == -1) {
> +        vshError(ctl, "%s", _("Invalid value for 'cellno', expecting an int"));

-1 is a valid int, but not a valid cellno, so --cellno=-1 would now give
a misleading message.

I also don't like the fact that you are changing the semantics of
vshCommandOptInt, but not the counterparts such as vshCommandOptUL.  I'd
rather see all the vshCommandOpt* functions have the same semantics
regarding their found parameter.

And, since some of the vshCommandOpt* return unsigned values, you can't
rely on -1 as a sentinel return value to indicate argument present but
invalid.  You'd have to go with something different, such as altering
the semantics of the found argument: instead of being a binary
TRUE/FALSE return (using TRUE/FALSE and int* is nasty anyways, because
we already have <stdbool.h> and could be using true/false and bool*
instead), let's instead have it be a ternary value:

found < 0 => argument was present, but invalid (not an integer); return
value is 0
found == 0 => argument was missing; return value is 0
found > 0 => argument was present and valid; return value is its value

but this means that all existing callers that pass NULL instead of
&found as the third argument are silently ignoring errors, at which
point, it seems like we should require a non-NULL found argument.  But
if we do that, we might as well go one step further, and swap the order
of the API entirely (to force ALL callers to check for errors):

int
vshCommandOptUL(const vshCmd *cmd, const char *name,
                unsigned long *value) ATTRIBUTE_NONNULL(3);

Return value is <0 on failure (*value untouched), 0 when option is
absent (*value untouched), and >0 on success (*value updated).  Swapping
the API like that also has the benefit that a client can specify a
non-zero default:

unsigned long value = 1024;
if (vshCommandOptUL(cmd, "arg", &value) < 0) {
    error; return FALSE;
}
use value

rather than the current code usage of checking whether a value was
supplied, and if not, supplying the default.

Yes, an API swap like this would be a much bigger change, but it seems
like it is cleaner in the long run (since invalid cellno can't be the
only case where passing a non-integer string gets silently ignored back
to the default integer value).

> +        if ((arg->data == end_p) || (*end_p!= 0)) {
>              num_found = FALSE;
> -        else
> +            res = -1;
> +        } else
>              num_found = TRUE;

Style nit: you used:

if (cond) {
   abc;
   def;
} else
   xyz;

But we prefer either:

if (!cond)
   xyz;
else {
   abc;
   def;
}

or:

if (cond) {
   abc;
   def;
} else {
   xyz;
}

since HACKING documents that an else clause should only ever omit braces
when the if clause also omitted braces, but an if clause can omit braces
even when the else clause requires them.

NACK as-is; but I agree that this is (at least one) real bug to be
fixed.  Does my idea for a broader rewrite of the vshCommandOpt*
function semantics make sense?

-- 
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/20110105/5b03d8d8/attachment-0001.sig>