On Wed, Jan 13, 2010 at 09:47:00AM +0100, Jiri Denemark wrote:
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.
You don't want todo that because it is fine to have a topology that is not fully populated with CPUs
+ 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.
Yes, there are a couple of places where that might be useful. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|