On 11/20/2015 10:22 AM, Peter Krempa wrote:
Finalize the refactor by adding the 'virDomainDefGetVCpusMax' getter and reusing it accross libvirt. --- src/conf/domain_conf.c | 22 +++++++++++++++------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/libxl/libxl_conf.c | 4 ++-- src/libxl/libxl_driver.c | 9 ++++++--- src/openvz/openvz_driver.c | 4 ++-- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_driver.c | 10 +++++----- src/qemu/qemu_process.c | 2 +- src/test/test_driver.c | 13 ++++++++----- src/vbox/vbox_common.c | 4 ++-- src/vmx/vmx.c | 16 +++++++++------- src/vz/vz_driver.c | 2 +- src/xen/xm_internal.c | 9 ++++++--- src/xenapi/xenapi_utils.c | 6 ++---- src/xenconfig/xen_common.c | 4 ++-- src/xenconfig/xen_sxpr.c | 8 ++++---- 17 files changed, 69 insertions(+), 50 deletions(-)
Even just after this patch, a cscope search on "->maxvcpus" returns: libxlDomainGetPerCPUStats virDomainDefHasVCpusOffline Cannot recall for sure, but can there be some sort of syntax-check for direct accesses (outside of domain_conf)? Just so there's no current upstream patches that escape someone's review...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6b16430..4e5b7b6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1450,6 +1450,13 @@ virDomainDefHasVCpusOffline(const virDomainDef *def)
This API accesses maxvcpus directly still: return def->vcpus < def->maxvcpus; Although I do note by the end of the entire patch series a number of these new API's access ->maxvcpus directly. Just seems 'safer' than any access other the "Set" vcpusmax function uses the accessor. I'll try to remember to point them out when I see them (let's see how well short term memory is working today!).
}
+unsigned int +virDomainDefGetVCpusMax(const virDomainDef *def)
s/VCpus/Vcpus/g (or VCPUs - whatever had been chosen)
+{ + return def->maxvcpus; +} + +
[...]
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 0223e94..44f76f2 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -3066,6 +3066,7 @@ virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virDomainDe bool scsi_present[4] = { false, false, false, false }; int scsi_virtualDev[4] = { -1, -1, -1, -1 }; bool floppy_present[2] = { false, false }; + unsigned int maxvcpus;
if (ctx->formatFileName == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -3181,15 +3182,16 @@ virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virDomainDe "'current'")); goto cleanup; } - if ((def->maxvcpus % 2 != 0 && def->maxvcpus != 1)) { + maxvcpus = virDomainDefGetVCpusMax(def); + if (maxvcpus % 2 != 0 && maxvcpus != 1) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Expecting domain XML entry 'vcpu' to be 1 or a " - "multiple of 2 but found %d"), - def->maxvcpus); + _("Expecting domain XML entry 'vcpu' to be an unsigned " + "integer (1 or a multiple of 2) but found %d"), + maxvcpus);
Error message thrashing ... patch 10 changed this message one way and then this patch changes it back. Doesn't really matter to me which way it goes, but I actually liked the way patch 10 says it rather than going back to this old format.
goto cleanup; }
- virBufferAsprintf(&buffer, "numvcpus = \"%d\"\n", def->maxvcpus); + virBufferAsprintf(&buffer, "numvcpus = \"%d\"\n", maxvcpus);
/* def:cpumask -> vmx:sched.cpu.affinity */ if (def->cpumask && virBitmapSize(def->cpumask) > 0) {
[...]
index a80e084..d40f959 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -504,10 +504,8 @@ createVMRecordFromXml(virConnectPtr conn, virDomainDefPtr def, else (*record)->memory_dynamic_max = (*record)->memory_static_max;
- if (def->maxvcpus) { - (*record)->vcpus_max = (int64_t) def->maxvcpus; - (*record)->vcpus_at_startup = (int64_t) def->vcpus; - } + (*record)->vcpus_max = (int64_t) virDomainDefGetVCpusMax(def); + (*record)->vcpus_at_startup = (int64_t) def->vcpus;
Hmmm... is this yet another hypervisor that allowed maxvcpus == 0 to mean get me the number of CPU's on the host? For which setting a 1 by default will change expectations? If patch 10 was where we forced maxvcpus to be at least 1, then perhaps the "if (def->maxvcpus)" check removal needs to be there instead - just so it's captured in the right place. ACK - with at least function name adjustment and accessor for libxlDomainGetPerCPUStats. Whether virDomainDefHasVCpusOffline changes or not is less important, although for consistency it probably should. John
if (def->onPoweroff) (*record)->actions_after_shutdown = actionShutdownLibvirt2XenapiEnum(def->onPoweroff); if (def->onReboot)