[libvirt] [PATCH v2 04/11] qemu-caps: Get host model directly from Qemu when available

[Jiri Denemark]

On Fri, Dec 09, 2016 at 14:38:33 -0500, Jason J. Herne wrote:
> From: "Collin L. Walling" <walling at linux.vnet.ibm.com>
> 
> When qmp query-cpu-model-expansion is available probe Qemu for its view of the
> host model. In kvm environments this can provide a more complete view of the
> host model because features supported by Qemu and Kvm can be considered.
> 
> Signed-off-by: Collin L. Walling <walling at linux.vnet.ibm.com>
> Signed-off-by: Jason J. Herne <jjherne at linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_capabilities.c | 180 ++++++++++++++++++++++++++++++++++++++++++-
>  src/qemu/qemu_capabilities.h |   1 +
>  2 files changed, 177 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 081afc5..793afcc 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -354,6 +354,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>                "gluster.debug_level",
>                "vhost-scsi",
>                "drive-iotune-group",
> +              "query-cpu-model-expansion",

We put an empty line after a group of 5 capabilities.

>      );
>  
>  
> @@ -397,6 +398,8 @@ struct _virQEMUCaps {
>      size_t ngicCapabilities;
>      virGICCapability *gicCapabilities;
>  
> +    qemuMonitorCPUModelInfoPtr hostCPUModelInfo;
> +
>      /* Anything below is not stored in the cache since the values are
>       * re-computed from the other fields or external data sources every
>       * time we probe QEMU or load the results from the cache.
...
> @@ -3052,6 +3123,85 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
>      virResetLastError();
>  }
>  
> +void
> +virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
> +                            virCapsPtr caps)
> +{

The following code should remain here:

    if (!caps)
        return;

    if (!virQEMUCapsGuestIsNative(caps->host.arch, qemuCaps->arch))
        goto error;

Otherwise probing s390 QEMU binary on non-s390 host would report an
error.

> +    switch (qemuCaps->arch) {
> +    case VIR_ARCH_S390:
> +    case VIR_ARCH_S390X:
> +        if (virQEMUCapsCopyCPUModelFromQEMU(qemuCaps) == 0)
> +            break;

It doesn't make sense to fallback to CopyCPUModelFromHost if
CopyCPUModelFromQEMU fails on s390.

> +
> +    default: virQEMUCapsCopyCPUModelFromHost(qemuCaps, caps);
> +    }
> +}
> +
> +
> +static int
> +virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> +                                xmlXPathContextPtr ctxt,
> +                                xmlNodePtr hostCPUNode)
> +{
> +    char *str = NULL;
> +    xmlNodePtr *featureNodes = NULL;
> +    xmlNodePtr oldnode = ctxt->node;
> +    int ret = -1;
> +    size_t i;
> +    int n;
> +
> +    if ((str = virXMLPropString(hostCPUNode, "model"))) {
> +        if (VIR_ALLOC(qemuCaps->hostCPUModelInfo) < 0)
> +            goto cleanup;
> +        if (VIR_STRDUP(qemuCaps->hostCPUModelInfo->name, str) < 0)
> +            goto cleanup;
> +    }

As I already noted in the previous version you should not allocate the
structure only if model attribute was set.

> +
> +    ctxt->node = hostCPUNode;
...
> @@ -3148,6 +3298,7 @@ virQEMUCapsLoadCache(virCapsPtr caps,
>      size_t i;
>      int n;
>      xmlNodePtr *nodes = NULL;
> +    xmlNodePtr node = NULL;

There's no need to initialize this variable.

>      xmlXPathContextPtr ctxt = NULL;
>      char *str = NULL;
>      long long int l;
> @@ -3247,6 +3398,11 @@ virQEMUCapsLoadCache(virCapsPtr caps,
>      }
>      VIR_FREE(str);
>  
> +    if ((node = virXPathNode("./hostCPU", ctxt)) &&
> +        virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, node) < 0)
> +        goto cleanup;
> +    node = NULL;

No need to reset it back to NULL either.

Anyway, you can move virXPathNode("./hostCPU", ctxt) inside
virQEMUCapsLoadHostCPUModelInfo.

> +
>      if (virQEMUCapsLoadCPUModels(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0 ||
>          virQEMUCapsLoadCPUModels(qemuCaps, ctxt, VIR_DOMAIN_VIRT_QEMU) < 0)
>          goto cleanup;
..
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index cbab879..71d2570 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -389,6 +389,7 @@ typedef enum {
>      QEMU_CAPS_GLUSTER_DEBUG_LEVEL, /* -drive gluster.debug_level={0..9} */
>      QEMU_CAPS_DEVICE_VHOST_SCSI, /* -device vhost-scsi-{ccw,pci} */
>      QEMU_CAPS_DRIVE_IOTUNE_GROUP, /* -drive throttling.group=<name> */

An empty line with /* 245 */ comment should be added here.

> +    QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION, /* qmp query-cpu-model-expansion */
>  
>      QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;

Jirka