[libvirt] [PATCH] virsh: fix no error when pass a count <= 0 to setvcpus
https://bugzilla.redhat.com/show_bug.cgi?id=1248277 When count <= 0, the client exit without set an error. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- tools/virsh-domain.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f7edeeb..b6da684 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6744,8 +6744,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 0) + if (vshCommandOptInt(ctl, cmd, "count", &count) < 0) goto cleanup; + if (count <= 0) { + vshError(ctl, _("Invalid value '%d' for number of virtual CPUs"), count); + goto cleanup; + } /* none of the options were specified */ if (!current && flags == 0) { -- 1.8.3.1
On Thu, 2015-07-30 at 10:57 +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1248277
When count <= 0, the client exit without set an error.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- tools/virsh-domain.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f7edeeb..b6da684 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6744,8 +6744,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;
- if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 0) + if (vshCommandOptInt(ctl, cmd, "count", &count) < 0) goto cleanup; + if (count <= 0) { + vshError(ctl, _("Invalid value '%d' for number of virtual CPUs"), count); + goto cleanup; + }
/* none of the options were specified */ if (!current && flags == 0) {
Nice catch, but I would go for the following diff instead: -----8<----- diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a61656f..4b8ebbd 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6819,7 +6819,7 @@ static bool cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - int count = 0; + unsigned int count = 0; bool ret = false; bool maximum = vshCommandOptBool(cmd, "maximum"); bool config = vshCommandOptBool(cmd, "config"); @@ -6846,8 +6846,15 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 0) + if (vshCommandOptUInt(ctl, cmd, "count", &count) < 0) + goto cleanup; + + if (count == 0) { + vshError(ctl, + _("Numeric value '%s' for <%s> option is malformed or out of range"), + "0", "count"); goto cleanup; + } /* none of the options were specified */ if (!current && flags == 0) { ----->8----- It's slightly more awkward, but his way we make sure the error message is the same whether the value used for the count option is "foo", "0" or "-20". vshCommandOptUInt() already uses that error message internally. Does it look good to you? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Thu, Jul 30, 2015 at 09:29:15 +0200, Andrea Bolognani wrote:
On Thu, 2015-07-30 at 10:57 +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1248277
When count <= 0, the client exit without set an error.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- tools/virsh-domain.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f7edeeb..b6da684 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6744,8 +6744,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;
- if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 0) + if (vshCommandOptInt(ctl, cmd, "count", &count) < 0) goto cleanup; + if (count <= 0) { + vshError(ctl, _("Invalid value '%d' for number of virtual CPUs"), count); + goto cleanup; + }
/* none of the options were specified */ if (!current && flags == 0) {
Nice catch, but I would go for the following diff instead:
-----8<----- diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a61656f..4b8ebbd 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6819,7 +6819,7 @@ static bool cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - int count = 0; + unsigned int count = 0; bool ret = false; bool maximum = vshCommandOptBool(cmd, "maximum"); bool config = vshCommandOptBool(cmd, "config"); @@ -6846,8 +6846,15 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;
- if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 0) + if (vshCommandOptUInt(ctl, cmd, "count", &count) < 0) + goto cleanup; + + if (count == 0) { + vshError(ctl, + _("Numeric value '%s' for <%s> option is malformed or out of range"),
Since we know that here we are trying to set 0 cpus which is wrong we can also say that explicitly in the error message rather than copying the generic one. Peter
On Thu, 2015-07-30 at 09:45 +0200, Peter Krempa wrote:
Since we know that here we are trying to set 0 cpus which is wrong we can also say that explicitly in the error message rather than copying the generic one.
Using the generic one means we don't have to add yet another translatable string, though. Or did you mean to use a completely different error message? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Thu, Jul 30, 2015 at 09:51:15 +0200, Andrea Bolognani wrote:
On Thu, 2015-07-30 at 09:45 +0200, Peter Krempa wrote:
Since we know that here we are trying to set 0 cpus which is wrong we can also say that explicitly in the error message rather than copying the generic one.
Using the generic one means we don't have to add yet another translatable string, though. Or did you mean to use a completely different error message?
Completely different. Something along: "Can't set 0 processors for a VM"
On Thu, 2015-07-30 at 10:05 +0200, Peter Krempa wrote:
On Thu, Jul 30, 2015 at 09:51:15 +0200, Andrea Bolognani wrote:
On Thu, 2015-07-30 at 09:45 +0200, Peter Krempa wrote:
Since we know that here we are trying to set 0 cpus which is wrong we can also say that explicitly in the error message rather than copying the generic one.
Using the generic one means we don't have to add yet another translatable string, though. Or did you mean to use a completely different error message?
Completely different. Something along: "Can't set 0 processors for a VM"
I see. I don't feel strongly either way, I just want to avoid having a bunch of very similar but non-identical messages; if you feel a custom error message is warranted in this situation then go right ahead. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Hi peter and andrea, I was fixing some issue in virsh client and find this old problem, can i post a new version of this patch ? Thanks, Luyao On 07/30/2015 04:47 PM, Andrea Bolognani wrote:
On Thu, Jul 30, 2015 at 09:51:15 +0200, Andrea Bolognani wrote:
Since we know that here we are trying to set 0 cpus which is wrong we can also say that explicitly in the error message rather than copying the generic one. Using the generic one means we don't have to add yet another
On Thu, 2015-07-30 at 09:45 +0200, Peter Krempa wrote: translatable string, though. Or did you mean to use a completely different error message? Completely different. Something along: "Can't set 0 processors for a VM" I see. I don't feel strongly either way, I just want to avoid having a bunch of very similar but non-identical messages; if you feel a custom error message is warranted in this situation
On Thu, 2015-07-30 at 10:05 +0200, Peter Krempa wrote: then go right ahead.
Cheers.
On Wed, 2015-10-21 at 17:01 +0800, lhuang wrote:
Hi peter and andrea,
I was fixing some issue in virsh client and find this old problem, can i post a new version of this patch ?
Right. I guess you could use my version of the patch as a starting point for nicer handling of negative values, but make the error message when count == 0 more specific as Peter suggested. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 10/21/2015 05:28 PM, Andrea Bolognani wrote:
On Wed, 2015-10-21 at 17:01 +0800, lhuang wrote:
Hi peter and andrea,
I was fixing some issue in virsh client and find this old problem, can i post a new version of this patch ? Right.
I guess you could use my version of the patch as a starting point for nicer handling of negative values, but make the error message when count == 0 more specific as Peter suggested.
Ok, i will use the error peter suggested and the way you handling of negative values in a new version. Thanks a lot for your quick reply.
Cheers.
Luyao
On 07/30/2015 03:29 PM, Andrea Bolognani wrote:
On Thu, 2015-07-30 at 10:57 +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1248277
When count <= 0, the client exit without set an error.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- tools/virsh-domain.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f7edeeb..b6da684 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6744,8 +6744,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;
- if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 0) + if (vshCommandOptInt(ctl, cmd, "count", &count) < 0) goto cleanup; + if (count <= 0) { + vshError(ctl, _("Invalid value '%d' for number of virtual CPUs"), count); + goto cleanup; + }
/* none of the options were specified */ if (!current && flags == 0) { Nice catch, but I would go for the following diff instead:
-----8<----- diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a61656f..4b8ebbd 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6819,7 +6819,7 @@ static bool cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - int count = 0; + unsigned int count = 0; bool ret = false; bool maximum = vshCommandOptBool(cmd, "maximum"); bool config = vshCommandOptBool(cmd, "config"); @@ -6846,8 +6846,15 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;
- if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 0) + if (vshCommandOptUInt(ctl, cmd, "count", &count) < 0) + goto cleanup; + + if (count == 0) { + vshError(ctl, + _("Numeric value '%s' for <%s> option is malformed or out of range"), + "0", "count"); goto cleanup; + }
/* none of the options were specified */ if (!current && flags == 0) { ----->8-----
It's slightly more awkward, but his way we make sure the error message is the same whether the value used for the count option is "foo", "0" or "-20". vshCommandOptUInt() already uses that error message internally.
Does it look good to you?
I think a clear error is works for me, i am not good at the error message for a long time ;) Thanks a lot for your quick review.
Cheers.
Luyao
participants (4)
-
Andrea Bolognani -
lhuang -
Luyao Huang
-
Peter Krempa