From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v7 21/21] xl: vNUMA support
Date: Wed, 11 Mar 2015 15:12:06 +0000
Message-ID: <1426086726.21353.267.camel@citrix.com>
References: <1425905497-30835-1-git-send-email-wei.liu2@citrix.com>
	<1425905497-30835-22-git-send-email-wei.liu2@citrix.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: <1425905497-30835-22-git-send-email-wei.liu2@citrix.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: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

On Mon, 2015-03-09 at 12:51 +0000, Wei Liu wrote:

> +Each B<VNODE_CONFIG_OPTION> is a quoted key=value pair. Supported
> +B<VNODE_CONFIG_OPTION>s are:

Which of these are optional and which are mandatory? All mandatory?

> +    if (!xlu_cfg_get_long (config, "maxmem", &l, 0))
> +        max_memkb1 = l * 1024;

I think you should arrange that if the user has specified maxmem then it
has already been parsed and is available in b_info->max_memkb for use in
this function. Reparsing it just leaves scope to get out of sync.

In particular this doesn't handle the defaulting of maxmem to memory.

> +#define ABORT_IF_FAILED(str)                                            \
> +                do {                                                    \
> +                    if (endptr == value || val == ULONG_MAX) {          \

Looking at the uses, I think you mean str here where you have value, no?

Given that you exit() here I think you could write this as a helper
function which did the parsing for you too, i.e. folds in the strtoul.

> +    if (total_cpus != b_info->max_vcpus) {
> +        fprintf(stderr, "xl: vnuma vcpus and maxvcpus= mismatch\n");
> +        exit(1);
> +    }

Can't we do as we do with memory here and set max_vcpus if it isn't
already configured?