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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

> @@ -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.

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

> +    if (STR_HAS_PREFIX(str, "all")) {
>          libxl_bitmap_set_any(cpumap);
> -        return 0;
> +        goto out;

I think this does the wrong thing with "^all".

> +    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.

Thanks,
Ian.

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