[libvirt] [PATCH v5 23/36] qemu_monitor: Introduce qemuMonitorCPUModelInfoNew

[Jiri Denemark]

On Sun, Dec 02, 2018 at 23:10:17 -0600, Chris Venteicher wrote:
> Use a helper function to allocate and initializes CPU Model Info structs.
> 
> Signed-off-by: Chris Venteicher <cventeic at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c |  2 +-
>  src/qemu/qemu_monitor.c      | 32 +++++++++++++++++++++++++++-----
>  src/qemu/qemu_monitor.h      |  2 ++
>  3 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 038e3ecf7a..cd685298e6 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3047,7 +3047,7 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
>          goto cleanup;
>      }
>  
> -    if (VIR_ALLOC(hostCPU) < 0)
> +    if (!(hostCPU = qemuMonitorCPUModelInfoNew(NULL)))
>          goto cleanup;
>  
>      if (!(hostCPU->name = virXMLPropString(hostCPUNode, "model"))) {

This makes no sense. You have qemuMonitorCPUModelInfoNew which takes a
model name, call it with NULL and set hostCPU->name manually two lines
later.

> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 84065c59dc..5effc74736 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3670,6 +3670,31 @@ qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
>  }
>  
>  
> +qemuMonitorCPUModelInfoPtr
> +qemuMonitorCPUModelInfoNew(const char *name)
> +{
> +    qemuMonitorCPUModelInfoPtr ret = NULL;
> +    qemuMonitorCPUModelInfoPtr model;
> +
> +    if (VIR_ALLOC(model) < 0)
> +        return NULL;
> +
> +    model->name = NULL;
> +    model->nprops = 0;
> +    model->props = NULL;
> +    model->migratability = false;

VIR_ALLOC() clears the memory, which is why you won't see any code
resetting anything to 0 (NULL, false) after allocation. Please, drop
this part, it will likely become incomplete once a new field is added
into the structure anyway.

> +
> +    if (VIR_STRDUP(model->name, name) < 0)
> +        goto cleanup;
> +
> +    VIR_STEAL_PTR(ret, model);
> +
> + cleanup:
> +    qemuMonitorCPUModelInfoFree(model);
> +    return ret;
> +}
> +
> +
>  void
>  qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info)
>  {
> @@ -3693,18 +3718,15 @@ qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info)
>  qemuMonitorCPUModelInfoPtr
>  qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig)
>  {
> -    qemuMonitorCPUModelInfoPtr copy;
> +    qemuMonitorCPUModelInfoPtr copy = NULL;
>      size_t i;
>  
> -    if (VIR_ALLOC(copy) < 0)
> +    if (!orig || !(copy = qemuMonitorCPUModelInfoNew(orig->name)))

I think the !orig part is not really necessary here, unless you
explicitly want to support qemuMonitorCPUModelInfoCopy(NULL). But if
that's the case, the change should go into separate patch with a
description why it is useful. This way it's hidden in an unrelated
patch.

>          goto error;
>  
>      if (VIR_ALLOC_N(copy->props, orig->nprops) < 0)
>          goto error;
>  
> -    if (VIR_STRDUP(copy->name, orig->name) < 0)
> -        goto error;
> -
>      copy->migratability = orig->migratability;
>      copy->nprops = orig->nprops;
>  
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 66bfdb0e5c..d486af201a 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1042,6 +1042,8 @@ int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
>  
>  void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info);
>  
> +qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoNew(const char *name);
> +
>  qemuMonitorCPUModelInfoPtr
>  qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig);

Looking at all users of qemuMonitorCPUModelInfoPtr I can see the new
function should also be used in qemuMonitorJSONGetCPUModelExpansion.

Jirka