Re: [Xen-devel] [PATCH RESEND 02/12] xl: allow for node-wise specification of vcpu pinning

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On gio, 2013-11-07 at 18:17 +0000, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH RESEND 02/12] xl: allow for node-wise 
> specification of vcpu pinning"):
> > Making it possible to use something like the following:
> >  * "nodes:0-3": all pCPUs of nodes 0,1,2,3;
> >  * "nodes:0-3,^node:2": all pCPUS of nodes 0,1,3;
> >  * "1,nodes:1-2,^6": pCPU 1 plus all pCPUs of nodes 1,2
> >    but not pCPU 6;
> >  * ...
> 
> Thanks.  This parsing is a lot clearer now.
> 
Good to hear that, again (yep, you said this before in a previous review
of a different series still including this patch :-D).

> > @@ -59,6 +59,11 @@
> >          }                                                               \
> >      })
> >  
> > +#define STR_HAS_PREFIX( a, b )  \
> > +    ( strncmp(a, b, strlen(b)) == 0 )
> > +#define STR_SKIP_PREFIX( a, b ) \
> > +    ( STR_HAS_PREFIX(a, b) ? (a) += strlen(b) : NULL )
> 
> I think it might be worth making the type of STR_SKIP_PREFIX be
> explicitly boolean.  Eg,
>   +    ( STR_HAS_PREFIX(a, b) ? ((a) += strlen(b), 1) : 0 )
> 
> Since the returned pointer value isn't very helpful.
> 
Good point. Will do.

> > -static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
> > +static int parse_range(const char *str, unsigned long *a, unsigned long *b)
> > +{
> > +    char *nstr, *endptr;
> 
> Missing consts ?
> 
Is it? For instance, I'm using endptr like this:

 *a = *b = strtoul(str, &endptr, 10);

And the prototype of strtoul is:

 unsigned long int strtoul(const char *nptr, char **endptr, int base);

So it won't work, not for endptr at least. What I can do is the
following:

 const char *nstr;
 char *endptr;

And I will, if you think it's better.

> > +    if (STR_HAS_PREFIX(str, "all")) {
> >          libxl_bitmap_set_any(cpumap);
> > -        return 0;
> > +        goto out;
> 
> I think this does the wrong thing with "^all".
> 
Good point again. I'll fix that.

> > +    for (ptr = strtok_r(cpu, ",", &saveptr); ptr;
> > +         ptr = strtok_r(NULL, ",", &saveptr)) {
> 
> A minor style complaint: If you are going to split two of these three
> items onto their own line, please give them all their own line.
> 
You mean ptr should have its own line? Like this?

+    for (ptr = strtok_r(cpu, ",", &saveptr);
+         ptr;
+         ptr = strtok_r(NULL, ",", &saveptr)) {

If yes, I sure can do that, although the result really looks super
unpleasant to me (but that's a matter of taste, I guess).

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: signature.asc
Description: This is a digitally signed message part

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