A Full CPUModelInfo structure with props is sent to QEMU for expansion.

virQEMUCapsProbeQMPHostCPU migratability logic partitioned into new function
for clarity.

Signed-off-by: Chris Venteicher <cventeic@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_capabilities.c b/src/qemu/qemu_capabilities.c
index bd75f82e70..8c5ec4cc9a 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2460,6 +2460,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
                            qemuMonitorPtr mon,
                            bool tcg)
 {
+    qemuMonitorCPUModelInfoPtr input;
     qemuMonitorCPUModelInfoPtr migratable = NULL;
     qemuMonitorCPUModelInfoPtr nonMigratable = NULL;
     qemuMonitorCPUModelInfoPtr augmented = NULL;
@@ -2493,7 +2494,8 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
     else
         type = QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC;
 
-    if (qemuMonitorGetCPUModelExpansion(mon, type, model, true, &migratable) < 0)
+    if (!(input = qemuMonitorCPUModelInfoNew(model)) ||
+        qemuMonitorGetCPUModelExpansion(mon, type, true, input, &migratable) < 0)
         goto cleanup;
 
     if (!migratable) {
@@ -2502,8 +2504,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
     }
 
     /* Try to check migratability of each feature. */
-    if (qemuMonitorGetCPUModelExpansion(mon, type, model, false,
-                                        &nonMigratable) < 0)
+    if (qemuMonitorGetCPUModelExpansion(mon, type, false, input, &nonMigratable) < 0)
         goto cleanup;
 
     if (virQEMUCapsMigratablePropsDiff(migratable, nonMigratable, &augmented) < 0)
@@ -2513,6 +2514,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
     ret = 0;
 
  cleanup:
+    qemuMonitorCPUModelInfoFree(input);
     qemuMonitorCPUModelInfoFree(migratable);
     qemuMonitorCPUModelInfoFree(nonMigratable);
     qemuMonitorCPUModelInfoFree(augmented);
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.
+ *
+ * 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
+                               )
 {
     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.h b/src/qemu/qemu_monitor.h
index d0c54f60a9..78ad8ae428 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1036,9 +1036,10 @@ typedef enum {
 
 int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
                                     qemuMonitorCPUModelExpansionType type,
-                                    const char *model_name,
                                     bool migratable,
-                                    qemuMonitorCPUModelInfoPtr *model_info);
+                                    qemuMonitorCPUModelInfoPtr input,
+                                    qemuMonitorCPUModelInfoPtr *expansion)
+    ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
 
 void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info);
 
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;
 
  cleanup:
+    VIR_FREE(expanded_model); /* Free structure but not reused contents */
     qemuMonitorCPUModelInfoFree(model_in);
+    qemuMonitorCPUModelInfoFree(expanded_model);
+
     virJSONValueFree(cmd);
     virJSONValueFree(reply);
     virJSONValueFree(json_model_in);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index c3abd0ddf0..942dd85ee1 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -367,10 +367,10 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
 
 int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
                                         qemuMonitorCPUModelExpansionType type,
-                                        const char *model_name,
                                         bool migratable,
-                                        qemuMonitorCPUModelInfoPtr *model_info)
-    ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5);
+                                        qemuMonitorCPUModelInfoPtr input,
+                                        qemuMonitorCPUModelInfoPtr *expansion)
+    ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
 
 int qemuMonitorJSONGetCommands(qemuMonitorPtr mon,
                                char ***commands)
diff --git a/tests/cputest.c b/tests/cputest.c
index 339119c63f..c438a8d09e 100644
--- a/tests/cputest.c
+++ b/tests/cputest.c
@@ -483,6 +483,7 @@ cpuTestMakeQEMUCaps(const struct data *data)
     virQEMUCapsPtr qemuCaps = NULL;
     qemuMonitorTestPtr testMon = NULL;
     qemuMonitorCPUModelInfoPtr model = NULL;
+    qemuMonitorCPUModelInfoPtr expansion = NULL;
     char *json = NULL;
 
     if (virAsprintf(&json, "%s/cputestdata/%s-cpuid-%s.json",
@@ -492,9 +493,12 @@ cpuTestMakeQEMUCaps(const struct data *data)
     if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true)))
         goto error;
 
+    if (!(model = qemuMonitorCPUModelInfoNew("host")))
+        goto cleanup;
+
     if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon),
                                         QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC,
-                                        "host", true, &model) < 0)
+                                        true, model, &expansion) < 0)
         goto error;
 
     if (!(qemuCaps = virQEMUCapsNew()))
@@ -506,8 +510,8 @@ cpuTestMakeQEMUCaps(const struct data *data)
         virQEMUCapsSet(qemuCaps, QEMU_CAPS_QUERY_CPU_DEFINITIONS);
 
     virQEMUCapsSetArch(qemuCaps, data->arch);
-    virQEMUCapsSetCPUModelInfo(qemuCaps, VIR_DOMAIN_VIRT_KVM, model);
-    model = NULL;
+    virQEMUCapsSetCPUModelInfo(qemuCaps, VIR_DOMAIN_VIRT_KVM, expansion);
+    expansion = NULL;
 
     if (virQEMUCapsProbeQMPCPUDefinitions(qemuCaps,
                                           qemuMonitorTestGetMonitor(testMon),
@@ -516,6 +520,7 @@ cpuTestMakeQEMUCaps(const struct data *data)
 
  cleanup:
     qemuMonitorCPUModelInfoFree(model);
+    qemuMonitorCPUModelInfoFree(expansion);
     qemuMonitorTestFree(testMon);
     VIR_FREE(json);
 
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list