[libvirt] [PATCH v5 27/36] qemu_capabilities: Introduce CPUModelInfo to virCPUDef function

[Jiri Denemark]

On Sun, Dec 02, 2018 at 23:10:21 -0600, Chris Venteicher wrote:
> Move existing code to convert between cpu model info structures
> (qemuMonitorCPUModelInfoPtr into virCPUDef)
> into a reusable function.
> 
> The new function is used in this and future patches.
> 
> Signed-off-by: Chris Venteicher <cventeic at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 84 ++++++++++++++++++++++++++----------
>  src/qemu/qemu_capabilities.h |  3 ++
>  2 files changed, 64 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 8c5ec4cc9a..74f670459f 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
...
> @@ -3655,6 +3642,57 @@ virQEMUCapsLoadCache(virArch hostArch,
>  }
>  
>  
> +/* qemuMonitorCPUModelInfo name               => virCPUDef model
> + * qemuMonitorCPUModelInfo boolean properties => virCPUDef features
> + *
> + * migratable true: mark non-migratable features as disabled
> + *            false: allow all features as required

Our common function documentation format would be better.

> + */
> +virCPUDefPtr
> +virQEMUCapsCPUModelInfoToCPUDef(bool migratable, qemuMonitorCPUModelInfoPtr model)

Each parameter on a separate line please. And I'd put @migratable after
@model since @model is the primary parameter in the conversion and
@migratable just changes the way some features are converted.

> +{
> +    virCPUDefPtr cpu = NULL;
> +    virCPUDefPtr ret = NULL;
> +    size_t i;
> +
> +    if (!model || VIR_ALLOC(cpu) < 0)
> +        goto cleanup;
> +
> +    VIR_DEBUG("model->name= %s", NULLSTR(model->name));

I don't think this function would actually work as expected if either
model or model->name were NULL. At best we'd return NULL, i.e., error
without reporting any error message.

VIR_ALLOC() should go after the initial VIR_DEBUG.

> +
> +    if (VIR_STRDUP(cpu->model, model->name) < 0 ||
> +        VIR_ALLOC_N(cpu->features, model->nprops) < 0)
> +        goto cleanup;
> +
> +    cpu->nfeatures_max = model->nprops;
> +    cpu->nfeatures = 0;
> +
> +    for (i = 0; i < model->nprops; i++) {
> +        virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
> +        qemuMonitorCPUPropertyPtr prop = model->props + i;
> +
> +        if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
> +            continue;
> +
> +        if (VIR_STRDUP(feature->name, prop->name) < 0)
> +            goto cleanup;
> +
> +        if (!prop->value.boolean ||
> +            (migratable && prop->migratable == VIR_TRISTATE_BOOL_NO))
> +            feature->policy = VIR_CPU_FEATURE_DISABLE;
> +        else
> +            feature->policy = VIR_CPU_FEATURE_REQUIRE;
> +
> +        cpu->nfeatures++;
> +    }
> +
> +    VIR_STEAL_PTR(ret, cpu);
> +
> + cleanup:
> +    virCPUDefFree(cpu);
> +    return ret;
> +}
> +

Two empty lines between functions.

>  static void
>  virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
>                                    virBufferPtr buf,
...

Jirka