Re: [libvirt] [PATCH] Implement CPU topology support for QEMU driver
> ADD_ARG_LIT("-smp");
> - ADD_ARG_LIT(vcpus);
> + if ((qemuCmdFlags & QEMUD_CMD_FLAG_SMP_TOPOLOGY) && def->cpu) {
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> + virBufferVSprintf(&buf, "%lu", def->vcpus);
> +
> + if (def->cpu->sockets > 0)
> + virBufferVSprintf(&buf, ",sockets=%u",
def->cpu->sockets);
> +
> + if (def->cpu->cores > 0)
> + virBufferVSprintf(&buf, ",cores=%u",
def->cpu->cores);
> +
> + if (def->cpu->threads > 0)
> + virBufferVSprintf(&buf, ",threads=%u",
def->cpu->threads);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + goto no_memory;
> + }
> +
> + ADD_ARG(virBufferContentAndReset(&buf));
> + }
> + else {
> + char vcpus[50];
> + snprintf(vcpus, sizeof(vcpus), "%lu", def->vcpus);
> + ADD_ARG_LIT(vcpus);
> + }
Can you split this piece of code out into a separate method with a
signature like
static char *qemuBuildCPUStr(virDomainDefPtr def, int qemuCmdFlags);
The main qemuBuildCommandLine method is getting out of control :-)
With pleasure.
Also, in the case where 'def->cpu' is NULL, but
QEMUD_CMD_FLAG_SMP_TOPOLOGY
is available, I think we should explicitly set an arg based on
sockets=def->vcpus
cores=1
threads=1
so that we avoid relying on any changable QEMU default values.
Makes sense. I was also considering a check for vcpus == sockets * cores *
threads, but I'm not totally convinced it is a good idea.
> + do {
> + if (*p == '\0' || *p == ',')
> + goto syntax;
> +
> + if ((next = strchr(p, ',')))
> + next++;
> +
> + if (c_isdigit(*p)) {
> + int n;
> + if (virStrToLong_i(p, &end, 10, &n) < 0 ||
> + !end || (*end != ',' && *end != '\0'))
> + goto syntax;
> + dom->vcpus = n;
> + } else {
> + int i;
> + int n = -1;
> + for (i = 0; i < ARRAY_CARDINALITY(options); i++) {
> + if (STRPREFIX(p, options[i])) {
> + p += strlen(options[i]);
> + if (virStrToLong_i(p, &end, 10, &n) < 0 ||
> + !end || (*end != ',' && *end !=
'\0'))
> + goto syntax;
> + topology[i] = n;
> + break;
> + }
> + }
> +
> + if (n < 0)
> + goto syntax;
> + }
> + } while ((p = next));
If you strip off the initial CPU count value, then you can use
qemuParseCommandLineKeywords
to parse the reset of the name=value args.
Ah, cool, I didn't know about it. I guess because it's not used very often in
the code. BTW, would it make sense to extend the function to (optionally)
support parsing of any comma-separated list such as keyword,name=value,...?
The "keyword" parts would result in NULL value in retvalues array and the
"name=value" parts would behave normally.
Jirka