[libvirt] [PATCH v2] virsh: avoid missing zero value judgement in cmdBlkiotune

[Alex Jia]

On 07/28/2011 05:16 PM, Michal Privoznik wrote:
> On 28.07.2011 11:04, Alex Jia wrote:
>> * tools/virsh.c: avoid missing zero value judgement in cmdBlkiotune, 
>> when
>>    weight is equal to 0, the cmdBlkiotune will not raise any error 
>> information
>>    when judge weight value first time, and execute else branch to 
>> judge weight
>>    value again, strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, 
>> sizeof(temp->field))
>>    will be not executed for ever. However, if and only if 
>> param->field is equal
>>    to VIR_DOMAIN_BLKIO_WEIGHT, underlying 
>> qemuDomainSetBlkioParameters function
>>    will check whether weight value is in range [100, 1000].
>>
>> * how to reproduce?
>>
>>    % virsh blkiotune ${guestname} --weight 0
>>
>> Signed-off-by: Alex Jia<ajia at redhat.com>
>> ---
>>   tools/virsh.c |   11 +++++------
>>   1 files changed, 5 insertions(+), 6 deletions(-)
>>
>
> I liked the first verion more than this. Although it is not shown in 
> this snippet,
> nparams is initialized to zero
>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index 8bd22dc..512f2c6 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -4037,14 +4037,13 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * 
>> cmd)
>>           goto cleanup;
>>       }
>>
>> -    if (weight) {
>> -        nparams++;
>> -        if (weight<  0) {
>> -            vshError(ctl, _("Invalid value of %d for I/O weight"), 
>> weight);
>> -            goto cleanup;
>> -        }
>> +    if (weight<= 0) {
>> +        vshError(ctl, _("Invalid value of %d for I/O weight"), weight);
>> +        goto cleanup;
>>       }
>>
> and therefore doing this
>> +    nparams++;
>> +
> will invalidate this test.
>>       if (nparams == 0) {
>>           /* get the number of blkio parameters */
>>           if (virDomainGetBlkioParameters(dom, NULL,&nparams, flags) 
>> != 0) {
>
> The better way of dealing with this issue IMO is to switch from
> if (weight) {
> to test if 'weight' parameter was given (vshCommandOptInt() returns 
> >0) and after this
> check for impermissible values. And after the latter test (still 
> inside 'weight argument
> given') increment nparams:
>
> if (weight_given) {
>     if (weight<=0) {
>         report error; goto cleanup;
>     nparams++;
> }
>
>
> Michal
I aware of this , absolutely agree, thanks for your nice comment.

Regards,
Alex