[libvirt] [PATCH 11/34] conf: Replace read access to def->maxvcpus with accessor

[John Ferlan]

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)