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

[Michal Privoznik <mprivozn redhat com>]

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