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

[Osier Yang]

于 2011年01月06日 01:10, 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.

urgh, yes.

>
> 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:

agree, I thought like so when making the patch, but was not sure if
it would make easy things complicated.

>
> 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).

indeed, we do have more than one bug caused by this problem.

>
>> +        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.
>

good to known, thanks.

> 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?
>

+1, as a vote.

Regards
Osier