[libvirt] [PATCH 03/41] qemu: Use virDomainCapsCPUModels for cpuDefinitions

[John Ferlan]

On 08/12/2016 09:32 AM, Jiri Denemark wrote:
> The list of supported CPU models in domain capabilities is stored in
> virDomainCapsCPUModels. Let's use the same object for storing CPU models
> in QEMU capabilities.
> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 162 +++++++++++++++++++++++++++----------------
>  src/qemu/qemu_capabilities.h |  10 +--
>  src/qemu/qemu_command.c      |   8 ++-
>  src/qemu/qemu_monitor.c      |  12 +++-
>  src/qemu/qemu_monitor.h      |  10 ++-
>  src/qemu/qemu_monitor_json.c |  24 +++++--
>  src/qemu/qemu_monitor_json.h |   2 +-
>  tests/qemumonitorjsontest.c  |   8 +--
>  tests/qemuxml2argvtest.c     |  18 ++---
>  9 files changed, 164 insertions(+), 90 deletions(-)
> 

[..]

>  
>  
> -size_t virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
> -                                    char ***names)
> +int
> +virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
> +                             char ***names,
> +                             size_t *count)
>  {
> +    size_t i;
> +    char **models = NULL;
> +
> +    *count = 0;
>      if (names)
> -        *names = qemuCaps->cpuDefinitions;
> -    return qemuCaps->ncpuDefinitions;
> +        *names = NULL;
> +
> +    if (!qemuCaps->cpuDefinitions)
> +        return 0;
> +
> +    if (names && VIR_ALLOC_N(models, qemuCaps->cpuDefinitions->count) < 0)
> +        return -1;
> +

So if we don't have names, we don't get models and the following loop
will only add to models if we've allocated it because we have names, right?

Thus could there be an optimization to set/return ->count if !names
prior to this check? e.g.

if (!names) {
    *count = qemuCaps->cpuDefinitions->count;
    return 0;
}

This works, but it's tough to read.

> +    for (i = 0; i < qemuCaps->cpuDefinitions->count; i++) {
> +        virDomainCapsCPUModelPtr cpu = qemuCaps->cpuDefinitions->models + i;
> +        if (models && VIR_STRDUP(models[i], cpu->name) < 0)
> +            goto error;
> +    }
> +
> +    if (names)
> +        *names = models;
> +    *count = qemuCaps->cpuDefinitions->count;
> +    return 0;
> +
> + error:
> +    virStringFreeListCount(models, i);
> +    return -1;
>  }
>  

[...]

> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4855,14 +4855,15 @@ int qemuMonitorJSONGetMachines(qemuMonitorPtr mon,
>  }
>  
>  
> -int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
> -                                     char ***cpus)
> +int
> +qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
> +                                 qemuMonitorCPUDefInfoPtr **cpus)
>  {
>      int ret = -1;
>      virJSONValuePtr cmd;
>      virJSONValuePtr reply = NULL;
>      virJSONValuePtr data;
> -    char **cpulist = NULL;
> +    qemuMonitorCPUDefInfoPtr *cpulist = NULL;
>      int n = 0;
>      size_t i;
>  
> @@ -4898,13 +4899,18 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>          goto cleanup;
>      }
>  
> -    /* null-terminated list */
> -    if (VIR_ALLOC_N(cpulist, n + 1) < 0)
> +    if (VIR_ALLOC_N(cpulist, n) < 0)
>          goto cleanup;
>  
>      for (i = 0; i < n; i++) {
>          virJSONValuePtr child = virJSONValueArrayGet(data, i);
>          const char *tmp;
> +        qemuMonitorCPUDefInfoPtr cpu;
> +
> +        if (VIR_ALLOC(cpu) < 0)
> +            goto cleanup;
> +
> +        cpulist[i] = cpu;
>  
>          if (!(tmp = virJSONValueObjectGetString(child, "name"))) {
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -4912,7 +4918,7 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>              goto cleanup;
>          }
>  
> -        if (VIR_STRDUP(cpulist[i], tmp) < 0)
> +        if (VIR_STRDUP(cpu->name, tmp) < 0)
>              goto cleanup;
>      }
>  
> @@ -4921,7 +4927,11 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>      cpulist = NULL;
>  
>   cleanup:
> -    virStringFreeList(cpulist);
> +    if (ret < 0 && cpulist) {

I think "ret < 0" is superfluous... Coverity whines too

> +        for (i = 0; i < n; i++)
> +            qemuMonitorCPUDefInfoFree(cpulist[i]);
> +        VIR_FREE(cpulist);
> +    }
>      virJSONValueFree(cmd);
>      virJSONValueFree(reply);
>      return ret;

[...]