[libvirt] [PATCH v4 1/8] qemu_monitor: helper functions for CPU models

[Jiri Denemark]

On Wed, Jul 17, 2019 at 10:03:22 -0400, Collin Walling wrote:
> Refactor some code in qemuMonitorJSONGetCPUModelExpansion to be
> later used for the comparison and baseline functions.

You're mixing refactoring with adding new functionality. Splitting such
changes into separate patches is better. Moving some parts of existing
functions into new standalone functions is easier to review if the new
code matches the old one. It's not a big deal in this case, but since
you'll need to respin the series for other issues, you can also split
this patch.

> 
> This does not alter any functionality.
> 
> Signed-off-by: Collin Walling <walling at linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk at linux.ibm.com>
> Reviewed-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
> ---
>  src/qemu/qemu_monitor_json.c | 173 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 121 insertions(+), 52 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 8723ff4..f6bf7f2 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5616,6 +5616,120 @@ qemuMonitorJSONParseCPUModelProperty(const char *key,
>      return 0;
>  }
>  
> +
> +static virJSONValuePtr
> +qemuMonitorJSONMakeCPUModel(const char *model_name,
> +                            size_t nprops,
> +                            virCPUFeatureDefPtr props,

Adding virCPUFeatureDefPtr parameter to this function should go into a
separate patch. And for consistency, I believe we tend to pass the
pointer first followed by number of items.

> +                            bool migratable)
> +{
> +    virJSONValuePtr value;
> +    virJSONValuePtr feats = NULL;
> +    size_t i;
> +
> +    if (!(value = virJSONValueNewObject()))
> +        goto cleanup;
> +
> +    if (virJSONValueObjectAppendString(value, "name", model_name) < 0)
> +        goto cleanup;
> +
> +    if (nprops || !migratable) {
> +        if (!(feats = virJSONValueNewObject()))
> +            goto cleanup;
> +
> +        for (i = 0; i < nprops; i++) {
> +            char *name = props[i].name;
> +            bool enabled = false;
> +
> +            /* policy may be reported as -1 if the CPU def is a host model */
> +            if (props[i].policy == VIR_CPU_FEATURE_REQUIRE ||
> +                props[i].policy == VIR_CPU_FEATURE_FORCE ||
> +                props[i].policy == -1)
> +                enabled = true;

The naming is quite confusing. The virCPUDef structure contains an array
of features and you call it "props" while QEMU calls them "props" in the
QMP protocol and your variable containing them is called "feats".

> +
> +            if (virJSONValueObjectAppendBoolean(feats, name, enabled) < 0)
> +                goto cleanup;
> +        }
> +
> +        if (!migratable &&
> +            virJSONValueObjectAppendBoolean(feats, "migratable", false) < 0) {
> +            goto cleanup;
> +        }
> +
> +        if (virJSONValueObjectAppend(value, "props", feats) < 0)
> +            goto cleanup;
> +    }
> +
> +    return value;
> +
> + cleanup:

"cleanup" label is used if the code following it is shared between both
success and failure paths. Since it's not the case here, you should call
it "error".

> +    virJSONValueFree(value);
> +    virJSONValueFree(feats);
> +    return NULL;
> +}
> +
> +
> +static int
> +qemuMonitorJSONParseCPUModelData(virJSONValuePtr data,
> +                                 virJSONValuePtr *cpu_model,

cpu_model is not used anywhere outside this function, it's just a
temporary variable to store the container object for "model" and
"props".

> +                                 virJSONValuePtr *cpu_props,
> +                                 const char **cpu_name,
> +                                 const char *cmd_name)

I'd move the cmd_name parameter to the second place after data so the
input parameters go first, followed by output parameters.

> +{
> +    if (!(*cpu_model = virJSONValueObjectGetObject(data, "model"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("%s reply data was missing 'model'"), cmd_name);
> +        return -1;
> +    }
> +
> +    if (!(*cpu_name = virJSONValueObjectGetString(*cpu_model, "name"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("%s reply data was missing 'name'"), cmd_name);
> +        return -1;
> +    }
> +
> +    /* Some older CPU models do not report properties */
> +    *cpu_props = virJSONValueObjectGetObject(*cpu_model, "props");

This change should go into a separate patch with an explanation. Also,
it only applies to s390. Missing "props" should still result in an error
on x86. I think the callers should decide whether this is fatal or not.

> +
> +    return 0;
> +}
> +
> +
> +static int
> +qemuMonitorJSONParseCPUModel(const char *cpu_name,
> +                             virJSONValuePtr cpu_props,
> +                             qemuMonitorCPUModelInfoPtr *model_info)
> +{
> +    qemuMonitorCPUModelInfoPtr machine_model = NULL;
> +    int ret = -1;
> +
> +    if (VIR_ALLOC(machine_model) < 0)
> +        goto cleanup;
> +
> +    if (VIR_STRDUP(machine_model->name, cpu_name) < 0)
> +        goto cleanup;
> +
> +    if (cpu_props) {

This is connected to making "props" optional.

> +        size_t nprops = virJSONValueObjectKeysNumber(cpu_props);
> +
> +        if (VIR_ALLOC_N(machine_model->props, nprops) < 0)
> +            goto cleanup;
> +
> +        if (virJSONValueObjectForeachKeyValue(cpu_props,
> +                                              qemuMonitorJSONParseCPUModelProperty,
> +                                              machine_model) < 0)
> +            goto cleanup;
> +    }
> +
> +    VIR_STEAL_PTR(*model_info, machine_model);
> +    ret = 0;
> +
> + cleanup:
> +    qemuMonitorCPUModelInfoFree(machine_model);
> +    return ret;
> +}
> +
> +
>  int
>  qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>                                      qemuMonitorCPUModelExpansionType type,
...

Jirka