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

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&lt;ajia(a)redhat.com&gt; > --- > 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