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.