10:34 p.m.
vcpupin will allow argument --vcpu as a signed number,
and pass it to virDomainPinVcpu directlly without
checking if this value is positive(valid).
virsh vcpupin r7 -1 0
error: numerical overflow: input too
large: 4294967295
This message is inaccurate, and the negative vcpu is
non-valuable. So forbid vcpu argument as a negative.
After patching, the result likes:
virsh vcpupin r6 -1
error: vcpupin: Invalid vCPU number.
virsh vcpupin r6 --cpulist 0-1
error: vcpupin: Missing vCPU
number in pin mode.
virsh vcpupin r6 --vcpu ABC
error: vcpupin: Invalid vCPU number
in query mode.
Signed-off-by: Jincheng Miao <jmiao(a)redhat.com>
---
tools/virsh-domain.c | 42 ++++++++++++++++++++++++------------------
1 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 84a6706..e302459 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5797,7 +5797,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
{
virDomainInfo info;
virDomainPtr dom;
- int vcpu = -1;
+ unsigned int vcpu;
const char *cpulist = NULL;
bool ret = false;
unsigned char *cpumap = NULL;
@@ -5809,6 +5809,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
bool live = vshCommandOptBool(cmd, "live");
bool current = vshCommandOptBool(cmd, "current");
bool query = false; /* Query mode if no cpulist */
+ int get_vcpu;
unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
@@ -5830,29 +5831,34 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
query = !cpulist;
- /* In query mode, "vcpu" is optional */
- if (vshCommandOptInt(cmd, "vcpu", &vcpu) < !query) {
- vshError(ctl, "%s",
- _("vcpupin: Invalid or missing vCPU number."));
- virDomainFree(dom);
- return false;
- }
-
- if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) {
- virDomainFree(dom);
- return false;
+ get_vcpu = vshCommandOptUInt(cmd, "vcpu", &vcpu);
+ if (get_vcpu <= 0) {
+ /* In query mode, "vcpu" is optional */
+ if (query && get_vcpu < 0) {
+ vshError(ctl, "%s",
+ _("vcpupin: Invalid vCPU number in query mode."));
+ goto cleanup;
+ }
+ /* In pin mode, "vcpu" is necessary */
+ if (!query) {
+ vshError(ctl, "%s",
+ _("vcpupin: Missing vCPU number in pin mode."));
+ goto cleanup;
+ }
}
if (virDomainGetInfo(dom, &info) != 0) {
vshError(ctl, "%s", _("vcpupin: failed to get domain
information."));
- virDomainFree(dom);
- return false;
+ goto cleanup;
}
if (vcpu >= info.nrVirtCpu) {
vshError(ctl, "%s", _("vcpupin: Invalid vCPU number."));
- virDomainFree(dom);
- return false;
+ goto cleanup;
+ }
+
+ if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) {
+ goto cleanup;
}
cpumaplen = VIR_CPU_MAPLEN(maxcpu);
@@ -5871,7 +5877,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
vshPrintExtra(ctl, "%s %s\n", _("VCPU:"), _("CPU
Affinity"));
vshPrintExtra(ctl, "----------------------------------\n");
for (i = 0; i < ncpus; i++) {
- if (vcpu != -1 && i != vcpu)
+ if (get_vcpu > 0 && i != vcpu)
continue;
vshPrint(ctl, "%4zu: ", i);
@@ -5880,8 +5886,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
if (!ret)
break;
}
-
}
+
VIR_FREE(cpumaps);
goto cleanup;
}
--
1.7.1
6:47 a.m.
On 05/29/14 05:34, Jincheng Miao wrote:
vcpupin will allow argument --vcpu as a signed number,
and pass it to virDomainPinVcpu directlly without
checking if this value is positive(valid).
> virsh vcpupin r7 -1 0
error: numerical overflow: input too large: 4294967295
This message is inaccurate, and the negative vcpu is
non-valuable. So forbid vcpu argument as a negative.
After patching, the result likes:
> virsh vcpupin r6 -1
error: vcpupin: Invalid vCPU number.
> virsh vcpupin r6 --cpulist 0-1
error: vcpupin: Missing vCPU number in pin mode.
> virsh vcpupin r6 --vcpu ABC
error: vcpupin: Invalid vCPU number in query mode.
Signed-off-by: Jincheng Miao <jmiao(a)redhat.com>
---
tools/virsh-domain.c | 42 ++++++++++++++++++++++++------------------
1 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 84a6706..e302459 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5797,7 +5797,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
{
virDomainInfo info;
virDomainPtr dom;
- int vcpu = -1;
+ unsigned int vcpu;
This needs to be initialized to 0 otherwise it might be used
uninitialized if the user didn's specify any argument.
const char *cpulist = NULL;
bool ret = false;
unsigned char *cpumap = NULL;
@@ -5809,6 +5809,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
bool live = vshCommandOptBool(cmd, "live");
bool current = vshCommandOptBool(cmd, "current");
bool query = false; /* Query mode if no cpulist */
+ int get_vcpu;
we usually use past tense for such names: s/get_cpu/got_cpu/
unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
@@ -5830,29 +5831,34 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
query = !cpulist;
- /* In query mode, "vcpu" is optional */
- if (vshCommandOptInt(cmd, "vcpu", &vcpu) < !query) {
- vshError(ctl, "%s",
- _("vcpupin: Invalid or missing vCPU number."));
- virDomainFree(dom);
- return false;
- }
-
- if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) {
- virDomainFree(dom);
- return false;
+ get_vcpu = vshCommandOptUInt(cmd, "vcpu", &vcpu);
Still not quite right ... vshCommandOptUInt currently wraps negative
numbers to their 2's complement and stores them in the uint.
This would currently work as:
$ tools/virsh vcpupin RHEL_nightly -4294967295
VCPU: CPU Affinity
----------------------------------
1: 0-3
We need to modify the vshCommandOptUInt helper to reject negative numbers.
+ if (get_vcpu <= 0) {
+ /* In query mode, "vcpu" is optional */
+ if (query && get_vcpu < 0) {
+ vshError(ctl, "%s",
+ _("vcpupin: Invalid vCPU number in query mode."));
+ goto cleanup;
+ }
+ /* In pin mode, "vcpu" is necessary */
+ if (!query) {
+ vshError(ctl, "%s",
+ _("vcpupin: Missing vCPU number in pin mode."));
+ goto cleanup;
+ }
}
if (virDomainGetInfo(dom, &info) != 0) {
vshError(ctl, "%s", _("vcpupin: failed to get domain
information."));
- virDomainFree(dom);
- return false;
+ goto cleanup;
}
if (vcpu >= info.nrVirtCpu) {
vshError(ctl, "%s", _("vcpupin: Invalid vCPU number."));
- virDomainFree(dom);
- return false;
+ goto cleanup;
+ }
+
+ if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) {
+ goto cleanup;
}
cpumaplen = VIR_CPU_MAPLEN(maxcpu);
@@ -5871,7 +5877,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
vshPrintExtra(ctl, "%s %s\n", _("VCPU:"), _("CPU
Affinity"));
vshPrintExtra(ctl, "----------------------------------\n");
for (i = 0; i < ncpus; i++) {
- if (vcpu != -1 && i != vcpu)
+ if (get_vcpu > 0 && i != vcpu)
This will be either 0 or 1 here, so the "> 0" part can be omitted.
continue;
vshPrint(ctl, "%4zu: ", i);
@@ -5880,8 +5886,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
if (!ret)
break;
}
-
}
+
VIR_FREE(cpumaps);
goto cleanup;
}
This patch results into breakage of the vcpupin test as outputs are
changing.
~/libvirt/tests $ VIR_TEST_DEBUG=2 ./vcpupin
--- exp 2014-05-29 13:17:40.170153617 +0200
+++ out 2014-05-29 13:17:40.166820284 +0200
@@ -1,2 +1,2 @@
-error: vcpupin: Invalid or missing vCPU number.
+error: vcpupin: Missing vCPU number in pin mode.
I'm going to post a v3 of this patch with the mistakes fixed and with
the additional patch to fix vshCommandOptUInt.
Also I'm going to improve the tests for all the cases I was able to
break the command.
Peter
9:51 a.m.
On 05/29/2014 05:47 AM, Peter Krempa wrote:
Still not quite right ... vshCommandOptUInt currently wraps negative
numbers to their 2's complement and stores them in the uint.
I recently tweaked virstring.c to provide virStrToLong_uip and friends
for rejecting negative input when parsing unsigned numbers.
The intent was that we need to make a case-by-case decision on which of
the two parsing styles to use - sometimes, wrapping -1 to max is
desirable, other times it is not.
We need to modify the vshCommandOptUInt helper to reject negative numbers.
It may mean that we need two flavors of vshCommandOptUInt.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:53 a.m.
On 05/29/14 16:51, Eric Blake wrote:
On 05/29/2014 05:47 AM, Peter Krempa wrote:
> Still not quite right ... vshCommandOptUInt currently wraps negative
> numbers to their 2's complement and stores them in the uint.
I recently tweaked virstring.c to provide virStrToLong_uip and friends
for rejecting negative input when parsing unsigned numbers.
The intent was that we need to make a case-by-case decision on which of
the two parsing styles to use - sometimes, wrapping -1 to max is
desirable, other times it is not.
>
> We need to modify the vshCommandOptUInt helper to reject negative numbers.
It may mean that we need two flavors of vshCommandOptUInt.
Actually we don't. All callers of vshCommandOptUInt don't expect to use
the wrap-to-2's-complement "feature". I've sent a V3 that does exactly
that.
3830
days inactive
3830
days old
3 comments
3 participants
participants (3)
-
Eric Blake -
Jincheng Miao -
Peter Krempa