[libvirt] [PATCH v5 24/36] qemu_monitor: Introduce qemuMonitorCPUModelInfo / JSON conversion

[Jiri Denemark]

On Sun, Dec 02, 2018 at 23:10:18 -0600, Chris Venteicher wrote:
> Conversion functions are used convert CPUModelInfo structs into
> QMP JSON and the reverse.
> 
> QMP JSON is of form:
> {"model": {"name": "IvyBridge", "props": {}}}
> 
> qemuMonitorCPUModelInfoBoolPropAdd is used to add boolean properties to
> CPUModelInfo struct.
> 
> qemuMonitorJSONGetCPUModelExpansion makes full use of conversions and
> propAdd in prep to support input of full cpu model in future.

I think this patch is doing too much at once and it's quite easy to get
lost between the FromJSON and ToJSON converters. Introducing each
converter in a separate patch would be better.

> 
> Signed-off-by: Chris Venteicher <cventeic at redhat.com>
> ---
>  src/qemu/qemu_monitor.c      |  24 ++++++
>  src/qemu/qemu_monitor.h      |   5 ++
>  src/qemu/qemu_monitor_json.c | 154 +++++++++++++++++++++++++----------
>  3 files changed, 138 insertions(+), 45 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 5effc74736..ddf4d96799 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3764,6 +3764,30 @@ qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig)
>  }
>  
>  
> +int
> +qemuMonitorCPUModelInfoBoolPropAdd(qemuMonitorCPUModelInfoPtr model,
> +                                   const char *prop_name,
> +                                   bool prop_value)

It looks a bit strange to have this wrapper for a code that would only
be in one place...

> +{
> +    int ret = -1;
> +    qemuMonitorCPUProperty prop;

Anyway, either put an empty line here or change this to

       qemuMonitorCPUProperty prop = {
           .type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN,
           .value.boolean = prop_value
       };

> +    prop.type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
> +    prop.value.boolean = prop_value;
> +
> +    if (VIR_STRDUP(prop.name, prop_name) < 0)
> +        goto cleanup;
> +
> +    if (VIR_APPEND_ELEMENT(model->props, model->nprops, prop) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(prop.name);
> +    return ret;
> +}
> +
> +
>  int
>  qemuMonitorGetCommands(qemuMonitorPtr mon,
>                         char ***commands)
...
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 5a806f6c0e..abfa4155ee 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5505,6 +5505,101 @@ qemuMonitorJSONParseCPUModelProperty(const char *key,
...
> +static qemuMonitorCPUModelInfoPtr
> +qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model)
> +{
> +    virJSONValuePtr cpu_props;
> +    qemuMonitorCPUModelInfoPtr model = NULL;
> +    qemuMonitorCPUModelInfoPtr ret = NULL;
> +    char const *cpu_name;
> +
> +    if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Parsed JSON reply missing 'name'"));
> +        goto cleanup;
> +    }
> +
> +    if (VIR_ALLOC(model) < 0)
> +        goto cleanup;
> +
> +    if (VIR_STRDUP(model->name, cpu_name) < 0)
> +        goto cleanup;
> +
> +    if ((cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) {
> +        if (VIR_ALLOC_N(model->props,
> +                        virJSONValueObjectKeysNumber(cpu_props)) < 0)
> +            goto cleanup;
> +
> +        if (virJSONValueObjectForeachKeyValue(cpu_props,
> +                                              qemuMonitorJSONParseCPUModelProperty,
> +                                              model) < 0)
> +            goto cleanup;
> +    }

The patch looks like a refactoring, but it actually changes a bit more.
Previously "props" was required, while now it is silently ignored if
missing. Perhaps it's a valid change, but it should not be hidden inside
a refactoring patch.

> +
> +    VIR_STEAL_PTR(ret, model);
> +
> + cleanup:
> +    qemuMonitorCPUModelInfoFree(model);
> +
> +    return ret;
> +}
> +
>  int
>  qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>                                      qemuMonitorCPUModelExpansionType type,
...

Jirka