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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Dario Faggioli writes ("[PATCH] 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;
>  * ...

I have some comments about this.

I think the idea is fine, and the syntax is not bad.

The documentation needs to explain the semantics more formally.

The parsing code is starting to be rather impenetrable.  Have you
considered parsing this with flex or maybe doing this with regexps or
something ?

If you do decide to keep it the way it is:

> +    ida = idb = strtoul(str, &endptr, 10);
> +    if (endptr == str)
> +        return EINVAL;

You need to check for overflow, too, everywhere you call strtoul.  It
may be easiest to wrap strtoul.

What if "long" is bigger than uint32_t ?  I think this can silently
overflow.  (Although perhaps people who write crazy strings of hex
asking for vcpu 2^40 or something deserve to silently lose.)

> -    if (!strcmp(cpu, "all")) {
> +    if (!strcmp(cpu, "all") || !strcmp(cpu, "nodes:all")) {
>          libxl_bitmap_set_any(cpumap);
>          return 0;

With your new "not" syntax, this needs to become not a special case.
Someone might write
   all,^7
and it should work.

I think your current code changes the meaning of
   ^7
from "not pcpu 7" to "empty set of pcpus", which is wrong.

> +        /* Are we dealing with cpus or nodes? */
> +        if (!strncmp(ptr, "node:", 5) || !strncmp(ptr, "nodes:", 6)) {
> +            isnode = true;
> +            ptr += 5 + (ptr[4] == 's');

Cripes.  Please, no.  None of these magic 4s and 5s and 6s, and the
ptr[4]=='s' thing is truly awful.

How about some macro STR_SKIP_PREFIX(), and two separate ifs,

I think this is starting to be complex enough that it ought to have
some in-tree test vectors.

Ian.

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