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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Dario Faggioli writes ("[PATCH v2 3/5] 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;
>  * ...
...
>  * code rearranged in order to look more simple to follow
>    and understand, as requested during review;

This is much better now.  Thank you!

> +static int parse_range(const char *str, unsigned long *a, unsigned long *b)
>  {
...
> +    if (endptr == str)
> +        return EINVAL;
> +    if (*a == ULONG_MAX)
> +        return ERANGE;

So parse_range returns errno value or 0.  This isn't mentioned
anywhere and is a bit unusual.

> +static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
> +{
...
> +    rc = libxl_node_bitmap_alloc(ctx, &node_cpumap, 0);
> +    if (rc) {
> +        fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
> +        return rc;

So update_cpumap_range returns a libxl error code.

> +    rc = parse_range(str, &ida, &idb);
> +    if (rc) {

But here you assign the errno value to an `rc' variable which holds a
libxl error code.  I think it would be better to make parse_range
return a libxl rc value.

I think also that parse_range doesn't notice if the specified range
contains junk between the second number and the comma ?

> +static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
> +{
...
> +        if (STR_HAS_PREFIX(ptr, "all") ||
> +            STR_HAS_PREFIX(ptr, "nodes:all")) {
> +            libxl_bitmap_set_any(cpumap);

Why not deal with all in parse_range ?  You'd avoid the second
special-case of "nodes:", and constructions like
   all,^3
would work.

> -vcpp_out:
> -    libxl_bitmap_dispose(&exclude_cpumap);
> +        rc = update_cpumap_range(ptr, cpumap);
> +        if (rc) {
> +            /* If failing, reset the cpumap and exit */
> +            libxl_bitmap_set_none(cpumap);

Surely the caller who gets a error should expect the cpumap to contain
arbitrary contents ?

Thanks,
Ian.

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