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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list