Re: [Qemu-devel] [PATCH V6 02/11] NUMA: split -numa option

[Laszlo Ersek <lersek@redhat.com>]

On 07/30/13 09:32, Wanlong Gao wrote:

> +static int numa_node_parse(NumaNodeOptions *opts)
> +{
> +    uint16_t nodenr;
> +    UInt16List *cpus = NULL;
> +
> +    if (opts->has_nodeid) {
> +        nodenr = opts->nodeid;
> +        if (nodenr >= MAX_NODES) {
> +            fprintf(stderr, "qemu: Max number of NUMA nodes reached: %"
> +                    PRIu16 "\n", nodenr);
> +            return -1;
> +        }
> +    } else {
> +        nodenr = nb_numa_nodes;
> +    }
> +
> +    for (cpus = opts->cpus; cpus; cpus = cpus->next) {
> +        bitmap_set(node_cpumask[nodenr], cpus->value->u16, 1);
> +    }
> +
> +    if (opts->has_mem) {
> +        int64_t mem_size;
> +        char *endptr;
> +        mem_size = strtosz(opts->mem, &endptr);
> +        if (mem_size < 0 || *endptr) {
> +            fprintf(stderr, "qemu: invalid numa mem size: %s\n", opts->mem);
> +            return -1;
> +        }
> +        node_mem[nodenr] = mem_size;
> +    }
> +
> +    return 0;
> +}
> +
> +static int numa_mem_parse(NumaMemOptions *opts)
> +{
> +    uint16_t nodenr;
> +    uint64_t mem_size;
> +
> +    if (opts->has_nodeid) {
> +        nodenr = opts->nodeid;
> +        if (nodenr >= MAX_NODES) {
> +            fprintf(stderr, "qemu: Max number of NUMA nodes reached: %"
> +                    PRIu16 "\n", nodenr);
> +            return -1;
> +        }
> +    } else {
> +        nodenr = nb_numa_mem_nodes;
> +    }
> +
> +    if (opts->has_size) {
> +        mem_size = opts->size;
> +        node_mem[nodenr] = mem_size;
> +    }
> +
> +    return 0;
> +}

Unless I'm missing something:

Just like "NumaMemOptions.size" (which has type 'size' in the JSON),
OptsVisitor could parse "NumaNodeOptions.mem" for you as well, if you
switched the latter's type to 'size' too.

... Hm, not really. This is probably a compatibility thing. Without any
specific suffix, strtosz() passes STRTOSZ_DEFSUFFIX_MB to
strtosz_suffix() (ie. the legacy optarg is expressed in megabytes, if
the user doesn't specify a suffix him/herself), while opts_type_size()
passes STRTOSZ_DEFSUFFIX_B.

I think this patch is good; if you adapt it to the ['UInt16'] ->
['uint16'] change I proposed for 01/11, you can add

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo