[libvirt] [PATCH v2 05/33] qemu: Refactor virQEMUCapsInitHostCPUModel

[John Ferlan]

On 02/15/2017 11:44 AM, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
> 
> Notes:
>     Version 2:
>     - no change
> 
>  src/qemu/qemu_capabilities.c | 109 ++++++++++++++++++++++---------------------
>  1 file changed, 55 insertions(+), 54 deletions(-)
> 

This is "visually" more than a refactor since you've specified an
initialization of cpu->fallback... That initialization gets essentially
the same value of 0 (ALLOW == 0) as would any VIR_ALLOC field, so it's
not a problem per se.  Still makes me wonder if there should have been
an "UNDEFINED" category...

My only other comment here is whether there is a concern that your error
path doesn't clear the qemuCaps->hostCPUModel.  It wasn't clear to me
whether this path can be called after libvirtd restart and if failure
would mean anything or not (perhaps the one reason I could think of
setting NULL previously).

ACK in principal - might be nice to mention why clearing hostCPUModel on
failure isn't required anymore.

John
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 399e31447..c5e57b4ab 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3041,37 +3041,36 @@ virQEMUCapsCPUFilterFeatures(const char *name,
>  }
>  
>  
> -static void
> -virQEMUCapsCopyCPUModelFromQEMU(virQEMUCapsPtr qemuCaps)
> +/**
> + * Returns  0 when host CPU model provided by QEMU was filled in qemuCaps,
> + *          1 when the caller should fall back to using virCapsPtr->host.cpu,
> + *         -1 on error.
> + */
> +static int
> +virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
> +                            virCPUDefPtr cpu)
>  {
> -    virCPUDefPtr cpu = NULL;
> -    qemuMonitorCPUModelInfoPtr modelInfo = NULL;
> +    qemuMonitorCPUModelInfoPtr modelInfo = qemuCaps->hostCPUModelInfo;
>      size_t i;
>  
> -    if (!(modelInfo = qemuCaps->hostCPUModelInfo)) {
> +    if (!modelInfo) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("missing host CPU model info from QEMU capabilities"
> -                         " for binary %s"), qemuCaps->binary);
> -        goto error;
> +                       _("missing host CPU model info from QEMU capabilities "
> +                         "for binary %s"),
> +                       qemuCaps->binary);
> +        return -1;
>      }
>  
> -    if (VIR_ALLOC(cpu) < 0)
> -        goto error;
> -
>      if (VIR_STRDUP(cpu->model, modelInfo->name) < 0 ||
>          VIR_ALLOC_N(cpu->features, modelInfo->nprops) < 0)
> -        goto error;
> +        return -1;
>  
>      cpu->nfeatures_max = modelInfo->nprops;
>      cpu->nfeatures = 0;
> -    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;
>  
>      for (i = 0; i < modelInfo->nprops; i++) {
>          if (VIR_STRDUP(cpu->features[i].name, modelInfo->props[i].name) < 0)
> -            goto error;
> +            return -1;
>  
>          if (modelInfo->props[i].supported)
>              cpu->features[i].policy = VIR_CPU_FEATURE_REQUIRE;
> @@ -3081,31 +3080,53 @@ virQEMUCapsCopyCPUModelFromQEMU(virQEMUCapsPtr qemuCaps)
>          cpu->nfeatures++;
>      }
>  
> -    qemuCaps->hostCPUModel = cpu;
> -    return;
> -
> - error:
> -    virCPUDefFree(cpu);
> -    qemuCaps->hostCPUModel = NULL;
> -    virResetLastError();
> +    return 0;
>  }
>  
>  
> -static void
> -virQEMUCapsCopyCPUModelFromHost(virQEMUCapsPtr qemuCaps,
> -                                virCapsPtr caps)
> +/**
> + * Returns  0 when host CPU model provided by QEMU was filled in qemuCaps,
> + *          1 when the caller should fall back to using virCapsPtr->host.cpu,
> + *         -1 on error.
> + */
> +static int
> +virQEMUCapsInitCPUModel(virQEMUCapsPtr qemuCaps,
> +                        virCPUDefPtr cpu)
> +{
> +    int ret = 1;
> +
> +    if (ARCH_IS_S390(qemuCaps->arch))
> +        ret = virQEMUCapsInitCPUModelS390(qemuCaps, cpu);
> +
> +    return ret;
> +}
> +
> +
> +void
> +virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
> +                            virCapsPtr caps)
>  {
>      virCPUDefPtr cpu = NULL;
> +    int rc;
>  
> -    if (caps->host.cpu && caps->host.cpu->model) {
> -        if (VIR_ALLOC(cpu) < 0)
> +    if (!caps || !virQEMUCapsGuestIsNative(caps->host.arch, qemuCaps->arch))
> +        return;
> +
> +    if (VIR_ALLOC(cpu) < 0)
> +        goto error;
> +
> +    cpu->type = VIR_CPU_TYPE_GUEST;
> +    cpu->mode = VIR_CPU_MODE_CUSTOM;
> +    cpu->match = VIR_CPU_MATCH_EXACT;
> +    cpu->fallback = VIR_CPU_FALLBACK_ALLOW;
> +
> +    if ((rc = virQEMUCapsInitCPUModel(qemuCaps, cpu)) < 0) {
> +        goto error;
> +    } else if (rc == 1) {
> +        VIR_DEBUG("No host CPU model info from QEMU; using host capabilities");
> +        if (!caps->host.cpu || !caps->host.cpu->model)
>              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 (virCPUDefCopyModelFilter(cpu, caps->host.cpu, true,
>                                       virQEMUCapsCPUFilterFeatures, NULL) < 0)
>              goto error;
> @@ -3116,30 +3137,10 @@ virQEMUCapsCopyCPUModelFromHost(virQEMUCapsPtr qemuCaps,
>  
>   error:
>      virCPUDefFree(cpu);
> -    qemuCaps->hostCPUModel = NULL;

^^^

>      virResetLastError();
>  }
>  
>  
> -void
> -virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
> -                            virCapsPtr caps)
> -{
> -    if (!caps || !virQEMUCapsGuestIsNative(caps->host.arch, qemuCaps->arch))
> -        return;
> -
> -    switch (qemuCaps->arch) {
> -    case VIR_ARCH_S390:
> -    case VIR_ARCH_S390X:
> -        virQEMUCapsCopyCPUModelFromQEMU(qemuCaps);
> -        break;
> -
> -    default:
> -        virQEMUCapsCopyCPUModelFromHost(qemuCaps, caps);
> -    }
> -}
> -
> -
>  static int
>  virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
>                                  xmlXPathContextPtr ctxt)
>