[PATCH v2] xl: convert vcpuid to signed in main_vcpupin()

[PATCH v2] xl: convert vcpuid to signed in main_vcpupin()

[Dario Faggioli]

As it needs to be, considering that we assign -1 to it (and
that, later, we check for it to be -1), to signify 'all vcpus'.

While at it, fix a coding style nit and improve error reporting.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Changes from v1:
 * improve the changelog
 * add a comment about why long instead of int
---
 tools/libxl/xl_cmdimpl.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index e6b9615..8a38077 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4638,7 +4638,12 @@ int main_vcpupin(int argc, char **argv)
     libxl_vcpuinfo *vcpuinfo;
     libxl_bitmap cpumap_hard, cpumap_soft;;
     libxl_bitmap *soft = &cpumap_soft, *hard = &cpumap_hard;
-    uint32_t vcpuid, domid;
+    uint32_t domid;
+    /*
+     * int would be enough for vcpuid, but we don't want to
+     * mess aroung range checking the return value of strtol().
+     */
+    long vcpuid;
     const char *vcpu, *hard_str, *soft_str;
     char *endptr;
     int opt, nb_cpu, nb_vcpu, rc = -1;
@@ -4656,10 +4661,10 @@ int main_vcpupin(int argc, char **argv)
     soft_str = (argc > optind+3) ? argv[optind+3] : NULL;
 
     /* Figure out with which vCPU we are dealing with */
-    vcpuid = strtoul(vcpu, &endptr, 10);
-    if (vcpu == endptr) {
+    vcpuid = strtol(vcpu, &endptr, 10);
+    if (vcpu == endptr || vcpuid < 0) {
         if (strcmp(vcpu, "all")) {
-            fprintf(stderr, "Error: Invalid argument.\n");
+            fprintf(stderr, "Error: Invalid argument %s as VCPU.\n", vcpu);
             goto out;
         }
         vcpuid = -1;
@@ -4725,12 +4730,11 @@ int main_vcpupin(int argc, char **argv)
 
     if (vcpuid != -1) {
         if (libxl_set_vcpuaffinity(ctx, domid, vcpuid, hard, soft)) {
-            fprintf(stderr, "Could not set affinity for vcpu `%u'.\n",
+            fprintf(stderr, "Could not set affinity for vcpu `%ld'.\n",
                     vcpuid);
             goto out;
         }
-    }
-    else {
+    } else {
         if (!(vcpuinfo = libxl_list_vcpu(ctx, domid, &nb_vcpu, &nb_cpu))) {
             fprintf(stderr, "libxl_list_vcpu failed.\n");
             goto out;

Re: [PATCH v2] xl: convert vcpuid to signed in main_vcpupin()

[Don Slutz]

On 09/04/14 07:44, Dario Faggioli wrote:
> As it needs to be, considering that we assign -1 to it (and
> that, later, we check for it to be -1), to signify 'all vcpus'.
>
> While at it, fix a coding style nit and improve error reporting.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Changes from v1:
>   * improve the changelog
>   * add a comment about why long instead of int

Looks good to me.

Reviewed-by: Don Slutz <dslutz@verizon.com>

    -Don Slutz


> ---
>   tools/libxl/xl_cmdimpl.c |   18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index e6b9615..8a38077 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4638,7 +4638,12 @@ int main_vcpupin(int argc, char **argv)
>       libxl_vcpuinfo *vcpuinfo;
>       libxl_bitmap cpumap_hard, cpumap_soft;;
>       libxl_bitmap *soft = &cpumap_soft, *hard = &cpumap_hard;
> -    uint32_t vcpuid, domid;
> +    uint32_t domid;
> +    /*
> +     * int would be enough for vcpuid, but we don't want to
> +     * mess aroung range checking the return value of strtol().
> +     */
> +    long vcpuid;
>       const char *vcpu, *hard_str, *soft_str;
>       char *endptr;
>       int opt, nb_cpu, nb_vcpu, rc = -1;
> @@ -4656,10 +4661,10 @@ int main_vcpupin(int argc, char **argv)
>       soft_str = (argc > optind+3) ? argv[optind+3] : NULL;
>   
>       /* Figure out with which vCPU we are dealing with */
> -    vcpuid = strtoul(vcpu, &endptr, 10);
> -    if (vcpu == endptr) {
> +    vcpuid = strtol(vcpu, &endptr, 10);
> +    if (vcpu == endptr || vcpuid < 0) {
>           if (strcmp(vcpu, "all")) {
> -            fprintf(stderr, "Error: Invalid argument.\n");
> +            fprintf(stderr, "Error: Invalid argument %s as VCPU.\n", vcpu);
>               goto out;
>           }
>           vcpuid = -1;
> @@ -4725,12 +4730,11 @@ int main_vcpupin(int argc, char **argv)
>   
>       if (vcpuid != -1) {
>           if (libxl_set_vcpuaffinity(ctx, domid, vcpuid, hard, soft)) {
> -            fprintf(stderr, "Could not set affinity for vcpu `%u'.\n",
> +            fprintf(stderr, "Could not set affinity for vcpu `%ld'.\n",
>                       vcpuid);
>               goto out;
>           }
> -    }
> -    else {
> +    } else {
>           if (!(vcpuinfo = libxl_list_vcpu(ctx, domid, &nb_vcpu, &nb_cpu))) {
>               fprintf(stderr, "libxl_list_vcpu failed.\n");
>               goto out;
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v2] xl: convert vcpuid to signed in main_vcpupin()

[Ian Campbell]

On Sat, 2014-09-06 at 09:30 -0400, Don Slutz wrote:
> On 09/04/14 07:44, Dario Faggioli wrote:
> > As it needs to be, considering that we assign -1 to it (and
> > that, later, we check for it to be -1), to signify 'all vcpus'.
> >
> > While at it, fix a coding style nit and improve error reporting.
> >
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> > ---
> > Changes from v1:
> >   * improve the changelog
> >   * add a comment about why long instead of int
> 
> Looks good to me.
> 
> Reviewed-by: Don Slutz <dslutz@verizon.com>

Acked + applied, thanks.