[libvirt] [PATCH v5 26/36] qemu_monitor: qemuMonitorGetCPUModelExpansion inputs and outputs CPUModelInfo

[Jiri Denemark]

On Sun, Dec 02, 2018 at 23:10:20 -0600, Chris Venteicher wrote:
> A Full CPUModelInfo structure with props is sent to QEMU for expansion.
> 
> virQEMUCapsProbeQMPHostCPU migratability logic partitioned into new function
> for clarity.

Did you forget to remove this sentence after splitting the patch in two
parts?

> 
> Signed-off-by: Chris Venteicher <cventeic at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c |  8 +++++---
>  src/qemu/qemu_monitor.c      | 31 ++++++++++++++++++++++++++-----
>  src/qemu/qemu_monitor.h      |  5 +++--
>  src/qemu/qemu_monitor_json.c | 16 +++++++++++-----
>  src/qemu/qemu_monitor_json.h |  6 +++---
>  tests/cputest.c              | 11 ++++++++---
>  6 files changed, 56 insertions(+), 21 deletions(-)
> 
...
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index ddf4d96799..bed6a2f90a 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3653,20 +3653,41 @@ qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr cpu)
>  }
>  
>  
> +/**
> + * qemuMonitorGetCPUModelExpansion:
> + * @mon:
> + * @type: qemuMonitorCPUModelExpansionType
> + * @migratable: Prompt QEMU to include non-migratable props for X86 models if false
> + * @input: Input model
> + * @expansion: Expanded output model (or NULL if QEMU rejects model or request)
> + *
> + * Re-represent @input CPU props using a new CPUModelInfo constructed
> + * by naming a STATIC or DYNAMIC model cooresponding to a set of properties and
> + * a FULL or PARTIAL (only deltas from model) property list.

There's only "static" or "full" expansion. The STATIC_FULL enum item is
just requesting a "full" expansion on the result of a previous "static"
expansion.

And CPU model expansion does not provide delta from model in any case.
It always reports all features. The "full" expansion as opposed to a
"static" one also reports aliases (it reports even more, but aliases is
what we care about). So for example if "full" expansion reports
"sse4-1", "sse4_1", and "sse4.1", "static" expansion will only report
one of them (I think it's "sse4.1", but I'm not sure which one is the
preferred name in QEMU).

> + *
> + * if @type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC
> + *   construct @expansion using STATIC model name and a PARTIAL (delta) property list
> + *
> + * if @type == QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL
> + *   construct @expansion using DYNAMIC model name and a FULL property list
> + *
> + * if @type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL
> + *   construct @expansion using STATIC model name and a FULL property list
> + */
>  int
>  qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
>                                  qemuMonitorCPUModelExpansionType type,
> -                                const char *model_name,
>                                  bool migratable,
> -                                qemuMonitorCPUModelInfoPtr *model_info)
> +                                qemuMonitorCPUModelInfoPtr input,
> +                                qemuMonitorCPUModelInfoPtr *expansion
> +                               )

I don't see a reason for shuffling the parameters. Changing names or
types or both is OK, but changing the order of parameters at the same
time makes this patch harder to follow.

>  {
>      VIR_DEBUG("type=%d model_name=%s migratable=%d",
> -              type, model_name, migratable);
> +              type, input->name, migratable);
>  
>      QEMU_CHECK_MONITOR(mon);
>  
> -    return qemuMonitorJSONGetCPUModelExpansion(mon, type, model_name,
> -                                               migratable, model_info);
> +    return qemuMonitorJSONGetCPUModelExpansion(mon, type, migratable, input, expansion);
>  }
>  
>  
...
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index abfa4155ee..96e9f0ea3c 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5600,12 +5600,13 @@ qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model)
>      return ret;
>  }
>  
> +
>  int
>  qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>                                      qemuMonitorCPUModelExpansionType type,
> -                                    const char *model_name,
>                                      bool migratable,
> -                                    qemuMonitorCPUModelInfoPtr *model_info)
> +                                    qemuMonitorCPUModelInfoPtr input,
> +                                    qemuMonitorCPUModelInfoPtr *expansion)
>  {
>      int ret = -1;
>      virJSONValuePtr json_model_in = NULL;
> @@ -5613,12 +5614,13 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>      virJSONValuePtr reply = NULL;
>      virJSONValuePtr data;
>      virJSONValuePtr cpu_model;
> +    qemuMonitorCPUModelInfoPtr expanded_model = NULL;
>      qemuMonitorCPUModelInfoPtr model_in = NULL;
>      const char *typeStr = "";
>  
> -    *model_info = NULL;
> +    *expansion = NULL;
>  
> -    if (!(model_in = qemuMonitorCPUModelInfoNew(model_name)))
> +    if (!(model_in = qemuMonitorCPUModelInfoCopy(input)))
>          goto cleanup;
>  
>      if (!migratable &&
> @@ -5684,13 +5686,17 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>          goto retry;
>      }
>  
> -    if (!(*model_info = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model)))
> +    if (!(expanded_model = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model)))
>          goto cleanup;
>  
> +    VIR_STEAL_PTR(*expansion, expanded_model);
>      ret = 0;

So instead of

    *expansion = qemuMonitor...();

you do

    expanded_model = qemuMonitor...();
    *expansion = expanded_model;
    expanded_model = NULL;

Why?

>  
>   cleanup:
> +    VIR_FREE(expanded_model); /* Free structure but not reused contents */
>      qemuMonitorCPUModelInfoFree(model_in);
> +    qemuMonitorCPUModelInfoFree(expanded_model);
> +

What reused contents are you talking about here? Why do you call
qemuMonitorCPUModelInfoFree on something which was just freed and is
thus guaranteed to be NULL? Not to mention that expanded_model is
guaranteed to be always NULL once we enter the cleanup part, which means
even the VIR_FREE does nothing.


>      virJSONValueFree(cmd);
>      virJSONValueFree(reply);
>      virJSONValueFree(json_model_in);

Jirka