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

[Alex Jia]

On 07/28/2011 05:55 PM, Michal Privoznik wrote:
> On 28.07.2011 11:26, Alex Jia wrote:
>> On 07/28/2011 05:04 PM, 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(-)
>>>
>>> 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;
>>> }
>>>
>>> + nparams++;
>>> +
>>> if (nparams == 0) {
>>> /* get the number of blkio parameters */
>>> if (virDomainGetBlkioParameters(dom, NULL,&nparams, flags) != 0) {
>> Hmm, the above patch will introduce new issue, if blkiotune without any
>> weight option,
>> vshError will also be hit, that is not we expected.
>>
>> The reason of root is vshCommandOptInt will assign 0 to weight if option
>> not found
>> and not required, so we need to specifically deal with 0 value, I will
>> renew consider this
>> issue, so cancel this patch.
> Actually,  vshCommandOptInt (and all virsh opts parsing functions), 
> touches
> given variable only iff found & valid. In all other cases the variable
> remains untouched. It's the initialization which set weight to 0.
>
> The reason for such return values is, we can firstly initialize variable,
> then call option parsing function over it which will change it iff
> everything's fine:
>
> int var = 10;
> if (vshCommandOptInt(cmd, "int-attr", &var) < 0) {
>     /* error, e.g. string value given */
>     goto cleanup;
> }
>
> /* here, var may be 10, iff int-attr was not given,
> or other value if it was */
>
> Or, if "int-att" is required, we just change the test:
>
> if (vshCommandOptInt(cmd, "int-attr", &var) <= 0) {
Hi Michal,
"if option not found and not required (@value untouched)", it means 
weight value is
still 0 (keep initial 0 value), and if we only want to get weight value,
_("Unable to parse integer parameter")) will be also hit, this is not we 
expected.

Thanks a lot,
Alex

>
> Perhaps this will help :)
>
> Michal