[libvirt] [PATCH 3/3] qemu: Generate -numa command line option

[Eric Blake]

On 11/06/2011 06:59 AM, Bharata B Rao wrote:
> qemu: Generate -numa option
>
> From: Bharata B Rao<bharata at linux.vnet.ibm.com>
>
> Add routines to generate -numa QEMU command line option based on
> <numa>  ...</numa>  XML specifications.
>
> Signed-off-by: Bharata B Rao<bharata at linux.vnet.ibm.com>

>
> +static int
> +qemuBuildNumaCPUArgStr(char *cpumask, virBufferPtr buf)
> +{
> +    int i, first, last;
> +    int cpuSet = 0;
> +

What happens if cpumask is all 0?

> +    for (i = 0; i<  VIR_DOMAIN_CPUMASK_LEN; i++) {
> +        if (cpumask[i]) {

This if branch is skipped,

> +            if (cpuSet)
> +                last = i;
> +            else {
> +                first = last = i;
> +                cpuSet = 1;
> +            }
> +        } else {
> +            if (!cpuSet)
> +                continue;

so this branch always continues,

> +            if (first == last)
> +                virBufferAsprintf(buf, "%d,", first);
> +            else
> +                virBufferAsprintf(buf, "%d-%d,", first, last);
> +            cpuSet = 0;
> +	}
> +    }
> +
> +    if (cpuSet) {

and this if is skipped,

> +        if (first == last)
> +            virBufferAsprintf(buf, "%d,", first);
> +        else
> +            virBufferAsprintf(buf, "%d-%d,", first, last);
> +    }
> +
> +    /* Remove the trailing comma */
> +    return virBufferTruncate(buf, 1);

meaning that nothing was appended to buf, and you are now stripping 
unknown text, rather than a comma you just added.  Do we need a sanity 
check to ensure that the cpumask specifies at least one cpu?  And if so, 
would that mask be better here, or up front at the xml parsing time?

> +}
> +
> +static int
> +qemuBuildNumaArgStr(const virDomainDefPtr def, virCommandPtr cmd)
> +{
> +    int i;
> +    char *node;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    for (i = 0; i<  def->cpu->ncells; i++) {
> +        virCommandAddArg(cmd, "-numa");
> +        virBufferAsprintf(&buf, "%s", "node");

More efficient as virBufferAddLit(&buf, "node"), or...

> +        virBufferAsprintf(&buf, ",nodeid=%d", def->cpu->cells[i].cellid);

merge these two into a single:

virBufferAsprintf(&buf, "node,nodeid=%d", ...);

> +        virBufferAsprintf(&buf, ",cpus=");

Again, with no % in the format string, it is more efficient to use 
virBufferAddLit.

> +
> +        if (qemuBuildNumaCPUArgStr(def->cpu->cells[i].cpumask,&buf))

Generally, we prefer an explicit < 0 comparison when checking for failure.

> +            goto error;
> +
> +        virBufferAsprintf(&buf, ",mems=%d", def->cpu->cells[i].mem);
> +

Why do we need to bother with stripping a trailing comma in 
qemuBuildNumaCPUArgStr, if we will just be adding a comma back again 
here as the very next statement?  You could skip all the hassle of 
adding virBufferTruncate by just transferring the comma out of this 
statement and into qemuBuildNumaCPUArgStr (that said, I still think 
virBufferTruncate will be a useful addition in other contexts).

> +        if (virBufferError(&buf))
> +            goto error;
> +
> +        node = virBufferContentAndReset(&buf);
> +        virCommandAddArg(cmd, node);
> +        VIR_FREE(node);

It's more efficient to replace these five lines with one:

virCommandAddArgBuffer(cmd, &buf);

> @@ -3414,6 +3482,9 @@ qemuBuildCommandLine(virConnectPtr conn,
>       virCommandAddArg(cmd, smp);
>       VIR_FREE(smp);
>
> +    if (def->cpu&&  def->cpu->ncells&&  qemuBuildNumaArgStr(def, cmd))

Again, explicit < 0 check when looking for errors.

> +<topology sockets="2" cores="4" threads="2"/>
> +<numa>
> +<cell cpus="0-7" mems="109550"/>
> +<cell cpus="8-15" mems="109550"/>

Of course, this will need tweaking to match any XML changes made in 1/3, 
but thanks for adding test cases!

Overall, I think we'll need a v3 (you may want to use git send-email 
--subject-prefix=PATCHv3; it wasn't very clear from the subject line 
that this was already a v2 series), but I like where it's heading.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org