[libvirt] [PATCH 26/41] qemu: Store host-model CPU in qemu capabilities

[John Ferlan]

On 08/12/2016 09:33 AM, Jiri Denemark wrote:
> Host capabilities provide libvirt's view of the host CPU, but for a
> useful support for host-model CPUs we really need a hypervisor's view of
> the CPU. And since the view can be differ with emulator, qemu
> capabilities is the best place to store the host CPU model.
> 
> This patch just copies the CPU model from host capabilities, but this
> will change in the future.
> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 63 +++++++++++++++++++++++++++++++++++++-------
>  src/qemu/qemu_capspriv.h     |  7 ++++-
>  tests/domaincapstest.c       | 10 +++----
>  tests/qemucapabilitiestest.c |  2 +-
>  tests/qemuxml2argvtest.c     |  7 +++--
>  tests/testutilsqemu.c        |  5 ++--
>  tests/testutilsqemu.h        |  3 ++-
>  7 files changed, 76 insertions(+), 21 deletions(-)
> 

I personally think this should be closer to patch 22 where the caps gets
introduced into virQEMUCapsNewForBinaryInternal, but it's not a review
requirement...

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 8f55fcc..97dc877 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -374,6 +374,7 @@ struct _virQEMUCaps {
>      virArch arch;
>  
>      virDomainCapsCPUModelsPtr cpuDefinitions;
> +    virCPUDefPtr cpuModel;

hostCpuModel ?

IOW: Some way to make it more clear to a casual reader and perhaps
easier to find via cscope

Also I note that this isn't being written out to the cache (e.g. no
change to virQEMUCapsFormatCache), so probably need to "note" that some
how. Furthermore, perhaps this should be moved after the gic stuff and
the note made that anything "after" this point isn't written out, rather
it's refetched during Load

>  
>      size_t nmachineTypes;
>      struct virQEMUCapsMachineType *machineTypes;
> @@ -2052,6 +2053,10 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps)
>              goto error;
>      }
>  
> +    if (qemuCaps->cpuModel &&
> +        !(ret->cpuModel = virCPUDefCopy(qemuCaps->cpuModel))
> +        goto error;
> +
>      if (VIR_ALLOC_N(ret->machineTypes, qemuCaps->nmachineTypes) < 0)
>          goto error;
>      ret->nmachineTypes = qemuCaps->nmachineTypes;
> @@ -2821,6 +2826,37 @@ int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps,
>  }
>  
>  
> +void
> +virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
> +                            virCapsHostPtr host)
> +{
> +    virCPUDefPtr cpu = NULL;
> +
> +    if (!virQEMUCapsGuestIsNative(host->arch, qemuCaps->arch))
> +        goto error;
> +
> +    if (host->cpu && host->cpu->model) {
> +        if (VIR_ALLOC(cpu) < 0)
> +            goto error;
> +
> +        cpu->sockets = cpu->cores = cpu->threads = 0;
> +        cpu->type = VIR_CPU_TYPE_GUEST;
> +        cpu->mode = VIR_CPU_MODE_CUSTOM;
> +        cpu->match = VIR_CPU_MATCH_EXACT;
> +
> +        if (virCPUDefCopyModel(cpu, host->cpu, true) < 0)
> +            goto error;
> +    }
> +
> +    qemuCaps->cpuModel = cpu;

This is leaked - eg. no virCPUDefFree in virQEMUCapsDispose

Since this is a void function it is possible to have a failure (above)
thus having qemuCaps->cpuModel be NULL... Store that knowledge away as
it becomes important later in patch 40 as processing will call
virQEMUCapsGetHostModel which returns this field.

> +    return;
> +
> + error:
> +    virCPUDefFree(cpu);
> +    qemuCaps->cpuModel = NULL;

Should we virResetLastError() since we don't care about the failure in
the two callers?

> +}
> +
> +
>  /*
>   * Parsing a doc that looks like
>   *

[...]