8:45 a.m.
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)