[libvirt] [PATCH 05/23] cpu: Use virDomainCapsCPUModelsPtr in cpu driver APIs

[John Ferlan]

On 10/04/2017 10:58 AM, Jiri Denemark wrote:
> All APIs which expect a list of CPU models supported by hypervisors were
> switched from char **models and int models to just accept a pointer to
> virDomainCapsCPUModels object stored in domain capabilities. This avoids
> the need to transform virDomainCapsCPUModelsPtr into a NULL-terminated
> list of model names and also allows the various cpu driver APIs to
> access additional details (such as its usability) about each CPU model.
> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/cpu/cpu.c                  | 78 +++++++++++++-----------------------
>  src/cpu/cpu.h                  | 28 +++++--------
>  src/cpu/cpu_arm.c              |  3 +-
>  src/cpu/cpu_ppc64.c            | 13 +++---
>  src/cpu/cpu_x86.c              | 25 +++++-------
>  src/libxl/libxl_capabilities.c |  2 +-
>  src/libxl/libxl_driver.c       |  2 +-
>  src/qemu/qemu_capabilities.c   | 63 ++++++------------------------
>  src/qemu/qemu_capabilities.h   |  6 +--
>  src/qemu/qemu_driver.c         |  2 +-
>  src/qemu/qemu_process.c        |  9 +----
>  src/test/test_driver.c         |  2 +-
>  tests/cputest.c                | 89 ++++++++++++++++++++++++++++++------------
>  13 files changed, 138 insertions(+), 184 deletions(-)
> 

[...]

> @@ -501,11 +491,10 @@ virCPUProbeHost(virArch arch)
>   * @cpus: list of host CPU definitions
>   * @ncpus: number of CPUs in @cpus
>   * @models: list of CPU models that can be considered for the baseline CPU
> - * @nmodels: number of CPU models in @models
>   * @migratable: requests non-migratable features to be removed from the result
>   *
>   * Computes the most feature-rich CPU which is compatible with all given
> - * host CPUs. If @models array is NULL, all models supported by libvirt will
> + * host CPUs. If @models is NULL, all models supported by libvirt will
>   * be considered when computing the baseline CPU model, otherwise the baseline
>   * CPU model will be one of the provided CPU @models.
>   *
> @@ -514,21 +503,20 @@ virCPUProbeHost(virArch arch)
>  virCPUDefPtr
>  cpuBaseline(virCPUDefPtr *cpus,
>              unsigned int ncpus,
> -            const char **models,
> -            unsigned int nmodels,
> +            virDomainCapsCPUModelsPtr models,
>              bool migratable)
>  {
>      struct cpuArchDriver *driver;
>      size_t i;
>  
> -    VIR_DEBUG("ncpus=%u, nmodels=%u", ncpus, nmodels);
> +    VIR_DEBUG("ncpus=%u", ncpus);

You could add models=%p...  not that it's necessary

>      if (cpus) {
>          for (i = 0; i < ncpus; i++)
>              VIR_DEBUG("cpus[%zu]=%p", i, cpus[i]);
>      }
>      if (models) {
> -        for (i = 0; i < nmodels; i++)
> -            VIR_DEBUG("models[%zu]=%s", i, NULLSTR(models[i]));
> +        for (i = 0; i < models->nmodels; i++)
> +            VIR_DEBUG("models[%zu]=%s", i, models->models[i].name);
>      }
>  

[...]

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 7ddc6cafd4..225cee4ef9 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c


[...]

> -int
> +virDomainCapsCPUModelsPtr
>  virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
> -                             virDomainVirtType type,
> -                             char ***names,
> -                             size_t *count)
> +                             virDomainVirtType type)
>  {
> -    size_t i;
> -    char **models = NULL;
> -    virDomainCapsCPUModelsPtr cpus;
> -
> -    *count = 0;
> -    if (names)
> -        *names = NULL;
> -
>      if (type == VIR_DOMAIN_VIRT_KVM)
> -        cpus = qemuCaps->kvmCPUModels;
> +        return qemuCaps->kvmCPUModels;
>      else
> -        cpus = qemuCaps->tcgCPUModels;
> +        return qemuCaps->tcgCPUModels;
>  
> -    if (!cpus)
> -        return 0;
> -
> -    if (names && VIR_ALLOC_N(models, cpus->nmodels) < 0)
> -        return -1;
> -
> -    for (i = 0; i < cpus->nmodels; i++) {
> -        virDomainCapsCPUModelPtr cpu = cpus->models + i;
> -        if (models && VIR_STRDUP(models[i], cpu->name) < 0)
> -            goto error;
> -    }
> -
> -    if (names)
> -        *names = models;
> -    *count = cpus->nmodels;
> -    return 0;
> -
> - error:
> -    virStringListFreeCount(models, i);
> -    return -1;
> +    return NULL;

Nice overall simplification, but how do we reach here?  Wouldn't
tcgCPUModels either be something or NULL. Previous code just returns 0
w/ @models and @nmodels untouched which would seemingly be the same with
these changes.

That would also say to me the "else" above would be unnecessary.


>  }
>  
>  
> @@ -3392,8 +3353,6 @@ virQEMUCapsInitCPUModelX86(virQEMUCapsPtr qemuCaps,
>      virCPUDataPtr data = NULL;
>      unsigned long long sigFamily = 0;
>      unsigned long long sigModel = 0;
> -    size_t nmodels = 0;
> -    char **models = NULL;
>      int ret = -1;
>      size_t i;
>  

Reviewed-by: John Ferlan <jferlan at redhat.com>

John