[libvirt] [PATCH V2] virsh: forbid negative vcpu argument to vcpupin.

[Peter Krempa]

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 at 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140529/2c9aaa71/attachment-0001.sig>