New function qemuMonitorJSONBuildCPUModelInfoFromJSON
created by extracting code from existing function qemuMonitorJSONGetCPUModelExpansion
to create a reusable function for extracting cpu model info from json.

Function qemuMonitorJSONGetCPUModelInfoFromJSON
  returns qemuMonitorCPUModelInfoPtr on success and NULL on failure.
---
 src/qemu/qemu_monitor_json.c | 78 +++++++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 57c2c4de0..4368aaaa0 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5337,6 +5337,57 @@ qemuMonitorJSONParseCPUModelProperty(const char *key,
     return 0;
 }
 
+/* model_json: {"model": {"name": "IvyBridge", "props": {}}}
+ */
+static qemuMonitorCPUModelInfoPtr
+qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr model_json)
+{
+    virJSONValuePtr cpu_model;
+    virJSONValuePtr cpu_props;
+    qemuMonitorCPUModelInfoPtr machine_model = NULL;
+    qemuMonitorCPUModelInfoPtr model = NULL;
+    char const *cpu_name;
+
+    if (!(cpu_model = virJSONValueObjectGetObject(model_json, "model"))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("query-cpu-model-expansion reply data was missing 'model'"));
+        goto cleanup;
+    }
+
+    if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("query-cpu-model-expansion reply data was missing 'name'"));
+        goto cleanup;
+    }
+
+    if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("query-cpu-model-expansion reply data was missing 'props'"));
+        goto cleanup;
+    }
+
+    if (VIR_ALLOC(machine_model) < 0)
+        goto cleanup;
+
+    if (VIR_STRDUP(machine_model->name, cpu_name) < 0)
+        goto cleanup;
+
+    if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0)
+        goto cleanup;
+
+    if (virJSONValueObjectForeachKeyValue(cpu_props,
+                                          qemuMonitorJSONParseCPUModelProperty,
+                                          machine_model) < 0)
+        goto cleanup;
+
+    VIR_STEAL_PTR(model, machine_model);
+
+ cleanup:
+    qemuMonitorCPUModelInfoFree(machine_model);
+
+    return model;
+}
+
 int
 qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
                                     qemuMonitorCPUModelExpansionType type,
@@ -5351,9 +5402,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
     virJSONValuePtr reply = NULL;
     virJSONValuePtr data;
     virJSONValuePtr cpu_model;
-    virJSONValuePtr cpu_props;
     qemuMonitorCPUModelInfoPtr machine_model = NULL;
-    char const *cpu_name;
     const char *typeStr = "";
 
     *model_info = NULL;
@@ -5426,30 +5475,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
         goto retry;
     }
 
-    if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("query-cpu-model-expansion reply data was missing 'name'"));
-        goto cleanup;
-    }
-
-    if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("query-cpu-model-expansion reply data was missing 'props'"));
-        goto cleanup;
-    }
-
-    if (VIR_ALLOC(machine_model) < 0)
-        goto cleanup;
-
-    if (VIR_STRDUP(machine_model->name, cpu_name) < 0)
-        goto cleanup;
-
-    if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0)
-        goto cleanup;
-
-    if (virJSONValueObjectForeachKeyValue(cpu_props,
-                                          qemuMonitorJSONParseCPUModelProperty,
-                                          machine_model) < 0)
+    if (!(machine_model = qemuMonitorJSONBuildCPUModelInfoFromJSON(data)))
         goto cleanup;
 
     ret = 0;
-- 
2.14.1

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

On 04/19/2018 12:06 AM, Chris Venteicher wrote:
> New function qemuMonitorJSONBuildCPUModelInfoFromJSON
> created by extracting code from existing function qemuMonitorJSONGetCPUModelExpansion
> to create a reusable function for extracting cpu model info from json.
> 
> Function qemuMonitorJSONGetCPUModelInfoFromJSON
>   returns qemuMonitorCPUModelInfoPtr on success and NULL on failure.
> ---
>  src/qemu/qemu_monitor_json.c | 78 +++++++++++++++++++++++++++++---------------
>  1 file changed, 52 insertions(+), 26 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 57c2c4de0..4368aaaa0 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5337,6 +5337,57 @@ qemuMonitorJSONParseCPUModelProperty(const char *key,
>      return 0;
>  }
>  
> +/* model_json: {"model": {"name": "IvyBridge", "props": {}}}
> + */
> +static qemuMonitorCPUModelInfoPtr
> +qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr model_json)
> +{
> +    virJSONValuePtr cpu_model;
> +    virJSONValuePtr cpu_props;
> +    qemuMonitorCPUModelInfoPtr machine_model = NULL;
> +    qemuMonitorCPUModelInfoPtr model = NULL;
> +    char const *cpu_name;
> +
> +    if (!(cpu_model = virJSONValueObjectGetObject(model_json, "model"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("query-cpu-model-expansion reply data was missing 'model'"));
> +        goto cleanup;
> +    }

Hmm... since you use this helper within functions other than expansion, you'll need a more
generic error message here.

or... this function could take an additional char *parameter that contains the name of 
the qmp command and you can append this to the error messages.

> +
> +    if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("query-cpu-model-expansion reply data was missing 'name'"));
> +        goto cleanup;
> +    }
> +
> +    if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("query-cpu-model-expansion reply data was missing 'props'"));
> +        goto cleanup;
> +    }
> +
> +    if (VIR_ALLOC(machine_model) < 0)
> +        goto cleanup;
> +
> +    if (VIR_STRDUP(machine_model->name, cpu_name) < 0)
> +        goto cleanup;
> +
> +    if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0)
> +        goto cleanup;
> +
> +    if (virJSONValueObjectForeachKeyValue(cpu_props,
> +                                          qemuMonitorJSONParseCPUModelProperty,
> +                                          machine_model) < 0)
> +        goto cleanup;
> +
> +    VIR_STEAL_PTR(model, machine_model);
> +
> + cleanup:
> +    qemuMonitorCPUModelInfoFree(machine_model);
> +
> +    return model;
> +}
> +
>  int
>  qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>                                      qemuMonitorCPUModelExpansionType type,
> @@ -5351,9 +5402,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>      virJSONValuePtr reply = NULL;
>      virJSONValuePtr data;
>      virJSONValuePtr cpu_model;
> -    virJSONValuePtr cpu_props;
>      qemuMonitorCPUModelInfoPtr machine_model = NULL;
> -    char const *cpu_name;
>      const char *typeStr = "";
>  
>      *model_info = NULL;
> @@ -5426,30 +5475,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>          goto retry;
>      }
>  
> -    if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("query-cpu-model-expansion reply data was missing 'name'"));
> -        goto cleanup;
> -    }
> -
> -    if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("query-cpu-model-expansion reply data was missing 'props'"));
> -        goto cleanup;
> -    }
> -
> -    if (VIR_ALLOC(machine_model) < 0)
> -        goto cleanup;
> -
> -    if (VIR_STRDUP(machine_model->name, cpu_name) < 0)
> -        goto cleanup;
> -
> -    if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0)
> -        goto cleanup;
> -
> -    if (virJSONValueObjectForeachKeyValue(cpu_props,
> -                                          qemuMonitorJSONParseCPUModelProperty,
> -                                          machine_model) < 0)
> +    if (!(machine_model = qemuMonitorJSONBuildCPUModelInfoFromJSON(data)))
>          goto cleanup;
>  
>      ret = 0;
> 

Not super important at all, but:

Since you're in the neighborhood, I think it would be helpful to clean up 
qemuMonitorJSONGetCPUModelExpansion a tiny bit and do a VIR_STEAL_PTR for model_info 
and machine_model here.

It should also be safe to remove the free on machine_model here, since both the 
allocation and freeing is now handled within your Build function.

-- 
Respectfully,
- Collin Walling

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

$SUBJ:

"qemu_monitor_json: Introduce qemuMonitorJSONBuildCPUModelInfoFromJSON"

On 04/19/2018 12:06 AM, Chris Venteicher wrote:
> New function qemuMonitorJSONBuildCPUModelInfoFromJSON
> created by extracting code from existing function qemuMonitorJSONGetCPUModelExpansion
> to create a reusable function for extracting cpu model info from json.
> 
> Function qemuMonitorJSONGetCPUModelInfoFromJSON
>   returns qemuMonitorCPUModelInfoPtr on success and NULL on failure.

Then in here, just indicate:

Extract cpu_model parsing code from qemuMonitorJSONGetCPUModelExpansion
into a separate helper for future reuse.

> ---
>  src/qemu/qemu_monitor_json.c | 78 +++++++++++++++++++++++++++++---------------
>  1 file changed, 52 insertions(+), 26 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 57c2c4de0..4368aaaa0 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5337,6 +5337,57 @@ qemuMonitorJSONParseCPUModelProperty(const char *key,
>      return 0;
>  }
>  

Use 2 blank/empty lines between functions - mostly a readability thing.

> +/* model_json: {"model": {"name": "IvyBridge", "props": {}}}
> + */
> +static qemuMonitorCPUModelInfoPtr
> +qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr model_json)

The '_json' is probably redundant.

> +{
> +    virJSONValuePtr cpu_model;
> +    virJSONValuePtr cpu_props;
> +    qemuMonitorCPUModelInfoPtr machine_model = NULL;
> +    qemuMonitorCPUModelInfoPtr model = NULL;
> +    char const *cpu_name;
> +
> +    if (!(cpu_model = virJSONValueObjectGetObject(model_json, "model"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("query-cpu-model-expansion reply data was missing 'model'"));

As a different suggestion from Collin...

Since qemuMonitorJSONGetCPUModelExpansion already fetches @cpu_model,
why not just pass virJSONValuePtr cpu_model. Then in patch3, fetch
cpu_model but use the error for the specific command.

then...

> +        goto cleanup;
> +    }
> +
> +    if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("query-cpu-model-expansion reply data was missing 'name'"));

s/query-cpu-model-expansion/cpu_model/

> +        goto cleanup;
> +    }
> +
> +    if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("query-cpu-model-expansion reply data was missing 'props'"));

s/query-cpu-model-expansion/cpu_model/

> +        goto cleanup;
> +    }
> +
> +    if (VIR_ALLOC(machine_model) < 0)
> +        goto cleanup;
> +
> +    if (VIR_STRDUP(machine_model->name, cpu_name) < 0)
> +        goto cleanup;
> +
> +    if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0)
> +        goto cleanup;
> +
> +    if (virJSONValueObjectForeachKeyValue(cpu_props,
> +                                          qemuMonitorJSONParseCPUModelProperty,
> +                                          machine_model) < 0)
> +        goto cleanup;
> +
> +    VIR_STEAL_PTR(model, machine_model);
> +
> + cleanup:
> +    qemuMonitorCPUModelInfoFree(machine_model);
> +
> +    return model;
> +}
> +
>  int
>  qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>                                      qemuMonitorCPUModelExpansionType type,
> @@ -5351,9 +5402,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>      virJSONValuePtr reply = NULL;
>      virJSONValuePtr data;
>      virJSONValuePtr cpu_model;
> -    virJSONValuePtr cpu_props;
>      qemuMonitorCPUModelInfoPtr machine_model = NULL;
> -    char const *cpu_name;
>      const char *typeStr = "";
>  
>      *model_info = NULL;
> @@ -5426,30 +5475,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>          goto retry;
>      }
>  
> -    if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("query-cpu-model-expansion reply data was missing 'name'"));
> -        goto cleanup;
> -    }
> -
> -    if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("query-cpu-model-expansion reply data was missing 'props'"));
> -        goto cleanup;
> -    }
> -
> -    if (VIR_ALLOC(machine_model) < 0)
> -        goto cleanup;
> -
> -    if (VIR_STRDUP(machine_model->name, cpu_name) < 0)
> -        goto cleanup;
> -
> -    if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0)
> -        goto cleanup;
> -
> -    if (virJSONValueObjectForeachKeyValue(cpu_props,
> -                                          qemuMonitorJSONParseCPUModelProperty,
> -                                          machine_model) < 0)
> +    if (!(machine_model = qemuMonitorJSONBuildCPUModelInfoFromJSON(data)))
>          goto cleanup;
>  
>      ret = 0;

Feel free to do the

    VIR_STEAL_PTR(*model_info, machine_model)

or

    if (!(*machine_info = ...()))

and get rid of machine_model from here.


John
> 

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