[libvirt] [PATCH v2] virsh: avoid missing zero value judgement in cmdBlkiotune
* 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(-) 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) { -- 1.7.1
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
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@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
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@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. Thanks, Alex
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@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) { Perhaps this will help :) Michal
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@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
participants (3)
-
ajia＠redhat.com -
Alex Jia
-
Michal Privoznik