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)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list