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

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

[Dario Faggioli]

No functional changes, it just looks more correct, considering
that at some point in the function we assign -1 to it (and
at some other later point 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>
---
 tools/libxl/xl_cmdimpl.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f1c136a..a29a579 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4601,8 +4601,9 @@ 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;
     const char *vcpu, *hard_str, *soft_str;
+    uint32_t domid;
+    long vcpuid;
     char *endptr;
     int opt, nb_cpu, nb_vcpu, rc = -1;
 
@@ -4619,10 +4620,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;
@@ -4688,12 +4689,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] xl: convert vcpuid to signed in main_vcpupin()

[Ian Campbell]

On Wed, 2014-08-20 at 17:36 +0200, Dario Faggioli wrote:
> No functional changes, it just looks more correct, considering
> that at some point in the function we assign -1 to it (and
> at some other later point we check for it to be -1),

I think that's more than just "looks more correct", it's just wrong to
compare an unsigned to -1, or to assign it.

And there may well be a functional change since gcc is no longer allowed
to assume that the number is +ve and therefore cannot discard some of
these checks any more. We are probably just lucky that gcc isn't doing
so already (and unlucky that it isn't generating a warning...)

>  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>
> ---
>  tools/libxl/xl_cmdimpl.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index f1c136a..a29a579 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4601,8 +4601,9 @@ 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;
>      const char *vcpu, *hard_str, *soft_str;
> +    uint32_t domid;
> +    long vcpuid;

I think an int would be sufficiently large for this.

Ian.

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2506 bytes --]

On mer, 2014-09-03 at 14:52 +0100, Ian Campbell wrote:
> On Wed, 2014-08-20 at 17:36 +0200, Dario Faggioli wrote:
> > No functional changes, it just looks more correct, considering
> > that at some point in the function we assign -1 to it (and
> > at some other later point we check for it to be -1),
> 
> I think that's more than just "looks more correct", it's just wrong to
> compare an unsigned to -1, or to assign it.
> 
> And there may well be a functional change since gcc is no longer allowed
> to assume that the number is +ve and therefore cannot discard some of
> these checks any more. We are probably just lucky that gcc isn't doing
> so already (and unlucky that it isn't generating a warning...)
> 
Right, and thanks for all the details. :-)

Should I change the changelog, if I repost?

> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> > ---
> >  tools/libxl/xl_cmdimpl.c |   14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index f1c136a..a29a579 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -4601,8 +4601,9 @@ 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;
> >      const char *vcpu, *hard_str, *soft_str;
> > +    uint32_t domid;
> > +    long vcpuid;
> 
> I think an int would be sufficiently large for this.
> 
The only reason why I used a long is that the function used to do the
actual conversion is strtol(), which returns a long. Strictly speaking,
if using an int, I think I should then be checking for the returned
value to be within [INT_MIN, INT_MAX] and error out if it's not.

That being said, we can probably use int and leave the range checking
alone, as, if someone tries to pin vCPU INT_MAX+1, he/she deserves all
the bad that will happen, but since I was polishing the code... :-P

Just let me know if you want it to be and int, and if you think the
range checking should be there, and I'll copy.

Thanks and regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Ian Campbell]

On Wed, 2014-09-03 at 17:20 +0200, Dario Faggioli wrote:
> On mer, 2014-09-03 at 14:52 +0100, Ian Campbell wrote:
> > On Wed, 2014-08-20 at 17:36 +0200, Dario Faggioli wrote:
> > > No functional changes, it just looks more correct, considering
> > > that at some point in the function we assign -1 to it (and
> > > at some other later point we check for it to be -1),
> > 
> > I think that's more than just "looks more correct", it's just wrong to
> > compare an unsigned to -1, or to assign it.
> > 
> > And there may well be a functional change since gcc is no longer allowed
> > to assume that the number is +ve and therefore cannot discard some of
> > these checks any more. We are probably just lucky that gcc isn't doing
> > so already (and unlucky that it isn't generating a warning...)
> > 
> Right, and thanks for all the details. :-)
> 
> Should I change the changelog, if I repost?

Yes, please.

> 
> > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> > > ---
> > >  tools/libxl/xl_cmdimpl.c |   14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > index f1c136a..a29a579 100644
> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -4601,8 +4601,9 @@ 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;
> > >      const char *vcpu, *hard_str, *soft_str;
> > > +    uint32_t domid;
> > > +    long vcpuid;
> > 
> > I think an int would be sufficiently large for this.
> > 
> The only reason why I used a long is that the function used to do the
> actual conversion is strtol(), which returns a long.

Can't it use atoi?

Ian.

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 3106 bytes --]

On mer, 2014-09-03 at 17:09 +0100, Ian Campbell wrote:
> On Wed, 2014-09-03 at 17:20 +0200, Dario Faggioli wrote:
> > 
> > > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> > > > ---
> > > >  tools/libxl/xl_cmdimpl.c |   14 +++++++-------
> > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > > index f1c136a..a29a579 100644
> > > > --- a/tools/libxl/xl_cmdimpl.c
> > > > +++ b/tools/libxl/xl_cmdimpl.c
> > > > @@ -4601,8 +4601,9 @@ 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;
> > > >      const char *vcpu, *hard_str, *soft_str;
> > > > +    uint32_t domid;
> > > > +    long vcpuid;
> > > 
> > > I think an int would be sufficiently large for this.
> > > 
> > The only reason why I used a long is that the function used to do the
> > actual conversion is strtol(), which returns a long.
> 
> Can't it use atoi?
> 
From atoi(3):

"The behavior is the same as strtol(nptr, NULL, 10); except that atoi()
does not detect errors."

While the current code uses some of the strtol() error handling
capabilities:

    /* Figure out with which vCPU we are dealing with */                                                              
    vcpuid = strtoul(vcpu, &endptr, 10);                                                                              
    if (vcpu == endptr) {                                                                                             
        if (strcmp(vcpu, "all")) {                                                                                    
            fprintf(stderr, "Error: Invalid argument.\n");                                                            
            goto out;                                                                                                 
        }                                                                                                             
        vcpuid = -1;                                                                                                  
    }

I can try to reorganize the code a bit but, without being sure that the
passed string, if not the string "all", is an actual number (which,
AFAIUI, is what strtol() gives me that atoi() doesn't), I don't think we
can be as precise in error detection/reporting as we are now (I just
double checked, and i=atoi("foo") means i becomes 0).

I can probably combine atoi() and (something like) isdigit(vcpu[0]) to
get to something similar, but at that point, I think I like strtol()
better.

Let me know.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Ian Campbell]

On Wed, 2014-09-03 at 18:42 +0200, Dario Faggioli wrote:
> On mer, 2014-09-03 at 17:09 +0100, Ian Campbell wrote:
> > On Wed, 2014-09-03 at 17:20 +0200, Dario Faggioli wrote:
> > > 
> > > > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> > > > > ---
> > > > >  tools/libxl/xl_cmdimpl.c |   14 +++++++-------
> > > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > > > index f1c136a..a29a579 100644
> > > > > --- a/tools/libxl/xl_cmdimpl.c
> > > > > +++ b/tools/libxl/xl_cmdimpl.c
> > > > > @@ -4601,8 +4601,9 @@ 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;
> > > > >      const char *vcpu, *hard_str, *soft_str;
> > > > > +    uint32_t domid;
> > > > > +    long vcpuid;
> > > > 
> > > > I think an int would be sufficiently large for this.
> > > > 
> > > The only reason why I used a long is that the function used to do the
> > > actual conversion is strtol(), which returns a long.
> > 
> > Can't it use atoi?
> > 
> From atoi(3):
> 
> "The behavior is the same as strtol(nptr, NULL, 10); except that atoi()
> does not detect errors."
> 
> While the current code uses some of the strtol() error handling
> capabilities:
> 
>     /* Figure out with which vCPU we are dealing with */                                                              
>     vcpuid = strtoul(vcpu, &endptr, 10);                                                                              
>     if (vcpu == endptr) {                                                                                             
>         if (strcmp(vcpu, "all")) {                                                                                    
>             fprintf(stderr, "Error: Invalid argument.\n");                                                            
>             goto out;                                                                                                 
>         }                                                                                                             
>         vcpuid = -1;                                                                                                  
>     }
> 
> I can try to reorganize the code a bit but, without being sure that the
> passed string, if not the string "all", is an actual number (which,
> AFAIUI, is what strtol() gives me that atoi() doesn't), I don't think we
> can be as precise in error detection/reporting as we are now (I just
> double checked, and i=atoi("foo") means i becomes 0).
> 
> I can probably combine atoi() and (something like) isdigit(vcpu[0]) to
> get to something similar, but at that point, I think I like strtol()
> better.

Yes, I'd forgotten that this wasn't a straight conversion.

It's probably easier to stick with long instead of farting around range
checking the result of strtol.

Ian.

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

[Andrew Cooper]

On 03/09/14 17:09, Ian Campbell wrote:
> On Wed, 2014-09-03 at 17:20 +0200, Dario Faggioli wrote:
>> On mer, 2014-09-03 at 14:52 +0100, Ian Campbell wrote:
>>> On Wed, 2014-08-20 at 17:36 +0200, Dario Faggioli wrote:
>>>> No functional changes, it just looks more correct, considering
>>>> that at some point in the function we assign -1 to it (and
>>>> at some other later point we check for it to be -1),
>>> I think that's more than just "looks more correct", it's just wrong to
>>> compare an unsigned to -1, or to assign it.
>>>
>>> And there may well be a functional change since gcc is no longer allowed
>>> to assume that the number is +ve and therefore cannot discard some of
>>> these checks any more. We are probably just lucky that gcc isn't doing
>>> so already (and unlucky that it isn't generating a warning...)
>>>
>> Right, and thanks for all the details. :-)
>>
>> Should I change the changelog, if I repost?
> Yes, please.
>
>>>> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
>>>> ---
>>>>  tools/libxl/xl_cmdimpl.c |   14 +++++++-------
>>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>>>> index f1c136a..a29a579 100644
>>>> --- a/tools/libxl/xl_cmdimpl.c
>>>> +++ b/tools/libxl/xl_cmdimpl.c
>>>> @@ -4601,8 +4601,9 @@ 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;
>>>>      const char *vcpu, *hard_str, *soft_str;
>>>> +    uint32_t domid;
>>>> +    long vcpuid;
>>> I think an int would be sufficiently large for this.
>>>
>> The only reason why I used a long is that the function used to do the
>> actual conversion is strtol(), which returns a long.
> Can't it use atoi?

No code written this decade should be using atoi() for input
sanitisation, given strto*() and friends in C99.

As a prefect example of why, try running `xenwatchdogd --help`

~Andrew

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

[Andrew Cooper]

On 03/09/14 17:09, Ian Campbell wrote:
> On Wed, 2014-09-03 at 17:20 +0200, Dario Faggioli wrote:
>> On mer, 2014-09-03 at 14:52 +0100, Ian Campbell wrote:
>>> On Wed, 2014-08-20 at 17:36 +0200, Dario Faggioli wrote:
>>>> No functional changes, it just looks more correct, considering
>>>> that at some point in the function we assign -1 to it (and
>>>> at some other later point we check for it to be -1),
>>> I think that's more than just "looks more correct", it's just wrong to
>>> compare an unsigned to -1, or to assign it.
>>>
>>> And there may well be a functional change since gcc is no longer allowed
>>> to assume that the number is +ve and therefore cannot discard some of
>>> these checks any more. We are probably just lucky that gcc isn't doing
>>> so already (and unlucky that it isn't generating a warning...)
>>>
>> Right, and thanks for all the details. :-)
>>
>> Should I change the changelog, if I repost?
> Yes, please.
>
>>>> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
>>>> ---
>>>>  tools/libxl/xl_cmdimpl.c |   14 +++++++-------
>>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>>>> index f1c136a..a29a579 100644
>>>> --- a/tools/libxl/xl_cmdimpl.c
>>>> +++ b/tools/libxl/xl_cmdimpl.c
>>>> @@ -4601,8 +4601,9 @@ 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;
>>>>      const char *vcpu, *hard_str, *soft_str;
>>>> +    uint32_t domid;
>>>> +    long vcpuid;
>>> I think an int would be sufficiently large for this.
>>>
>> The only reason why I used a long is that the function used to do the
>> actual conversion is strtol(), which returns a long.
> Can't it use atoi?

No code written this century should be using atoi() for input
conversion/sanitisation, given strto*() and friends in C99.

As a prefect example of why, try running `xenwatchdogd --help`

~Andrew