From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Jackson <Ian.Jackson@eu.citrix.com>
Subject: Re: [PATCH v2 3/5] xl: allow for node-wise
	specification of vcpu pinning
Date: Fri, 6 Sep 2013 17:37:04 +0100
Message-ID: <21034.1200.533286.543728@mariner.uk.xensource.com>
References: <20130906154725.10218.45008.stgit@hit-nxdomain.opendns.com>
	<20130906155540.10218.3642.stgit@hit-nxdomain.opendns.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <20130906155540.10218.3642.stgit@hit-nxdomain.opendns.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Dario Faggioli <dario.faggioli@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>, Ian Campbell <ian.campbell@citrix.com>, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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.