[libvirt] [PATCH 14/23] cputest: Separate QEMUCaps creation from cpuTestCPUIDJson

Jiri Denemark posted 23 patches 7 years, 7 months ago
There is a newer version of this series
[libvirt] [PATCH 14/23] cputest: Separate QEMUCaps creation from cpuTestCPUIDJson
Posted by Jiri Denemark 7 years, 7 months ago
To make the code reusable by other tests.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 tests/cputest.c | 97 +++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 60 insertions(+), 37 deletions(-)

diff --git a/tests/cputest.c b/tests/cputest.c
index 0a07a2da14..b72c17a168 100644
--- a/tests/cputest.c
+++ b/tests/cputest.c
@@ -460,6 +460,64 @@ cpuTestHasFeature(const void *arg)
 }
 
 
+typedef enum {
+    JSON_NONE,
+    JSON_HOST,
+    JSON_MODELS,
+} cpuTestCPUIDJson;
+
+#if WITH_QEMU && WITH_YAJL
+static virQEMUCapsPtr
+cpuTestMakeQEMUCaps(const struct data *data)
+{
+    virQEMUCapsPtr qemuCaps = NULL;
+    qemuMonitorTestPtr testMon = NULL;
+    qemuMonitorCPUModelInfoPtr model = NULL;
+    char *json = NULL;
+
+    if (virAsprintf(&json, "%s/cputestdata/%s-cpuid-%s.json",
+                    abs_srcdir, virArchToString(data->arch), data->host) < 0)
+        goto error;
+
+    if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true)))
+        goto error;
+
+    if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon),
+                                        QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC,
+                                        "host", true, &model) < 0)
+        goto error;
+
+    if (!(qemuCaps = virQEMUCapsNew()))
+        goto error;
+
+    virQEMUCapsSet(qemuCaps, QEMU_CAPS_KVM);
+    if (data->flags == JSON_MODELS)
+        virQEMUCapsSet(qemuCaps, QEMU_CAPS_QUERY_CPU_DEFINITIONS);
+
+    virQEMUCapsSetArch(qemuCaps, data->arch);
+    virQEMUCapsSetCPUModelInfo(qemuCaps, VIR_DOMAIN_VIRT_KVM, model);
+    model = NULL;
+
+    if (virQEMUCapsProbeQMPCPUDefinitions(qemuCaps,
+                                          qemuMonitorTestGetMonitor(testMon),
+                                          false) < 0)
+        goto error;
+
+ cleanup:
+    qemuMonitorCPUModelInfoFree(model);
+    qemuMonitorTestFree(testMon);
+    VIR_FREE(json);
+
+    return qemuCaps;
+
+ error:
+    virObjectUnref(qemuCaps);
+    qemuCaps = NULL;
+    goto cleanup;
+}
+#endif
+
+
 static int
 cpuTestCPUID(bool guest, const void *arg)
 {
@@ -670,52 +728,20 @@ cpuTestUpdateLive(const void *arg)
 }
 
 
-typedef enum {
-    JSON_NONE,
-    JSON_HOST,
-    JSON_MODELS,
-} cpuTestCPUIDJson;
-
 #if WITH_QEMU && WITH_YAJL
 static int
 cpuTestJSONCPUID(const void *arg)
 {
     const struct data *data = arg;
-    qemuMonitorCPUModelInfoPtr model = NULL;
     virQEMUCapsPtr qemuCaps = NULL;
     virCPUDefPtr cpu = NULL;
-    qemuMonitorTestPtr testMon = NULL;
-    char *json = NULL;
     char *result = NULL;
     int ret = -1;
 
-    if (virAsprintf(&json, "%s/cputestdata/%s-cpuid-%s.json",
-                    abs_srcdir, virArchToString(data->arch), data->host) < 0 ||
-        virAsprintf(&result, "cpuid-%s-json", data->host) < 0)
+    if (virAsprintf(&result, "cpuid-%s-json", data->host) < 0)
         goto cleanup;
 
-    if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true)))
-        goto cleanup;
-
-    if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon),
-                                        QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC,
-                                        "host", true, &model) < 0)
-        goto cleanup;
-
-    if (!(qemuCaps = virQEMUCapsNew()))
-        goto cleanup;
-
-    virQEMUCapsSet(qemuCaps, QEMU_CAPS_KVM);
-    if (data->flags == JSON_MODELS)
-        virQEMUCapsSet(qemuCaps, QEMU_CAPS_QUERY_CPU_DEFINITIONS);
-
-    virQEMUCapsSetArch(qemuCaps, data->arch);
-    virQEMUCapsSetCPUModelInfo(qemuCaps, VIR_DOMAIN_VIRT_KVM, model);
-    model = NULL;
-
-    if (virQEMUCapsProbeQMPCPUDefinitions(qemuCaps,
-                                          qemuMonitorTestGetMonitor(testMon),
-                                          false) < 0)
+    if (!(qemuCaps = cpuTestMakeQEMUCaps(data)))
         goto cleanup;
 
     if (VIR_ALLOC(cpu) < 0)
@@ -732,12 +758,9 @@ cpuTestJSONCPUID(const void *arg)
     ret = cpuTestCompareXML(data->arch, cpu, result);
 
  cleanup:
-    qemuMonitorCPUModelInfoFree(model);
     virObjectUnref(qemuCaps);
-    qemuMonitorTestFree(testMon);
     virCPUDefFree(cpu);
     VIR_FREE(result);
-    VIR_FREE(json);
     return ret;
 }
 #endif
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 14/23] cputest: Separate QEMUCaps creation from cpuTestCPUIDJson
Posted by John Ferlan 7 years, 7 months ago

On 10/04/2017 10:58 AM, Jiri Denemark wrote:
> To make the code reusable by other tests.
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  tests/cputest.c | 97 +++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 60 insertions(+), 37 deletions(-)
> 
> diff --git a/tests/cputest.c b/tests/cputest.c
> index 0a07a2da14..b72c17a168 100644
> --- a/tests/cputest.c
> +++ b/tests/cputest.c
> @@ -460,6 +460,64 @@ cpuTestHasFeature(const void *arg)
>  }
>  
>  
> +typedef enum {
> +    JSON_NONE,
> +    JSON_HOST,
> +    JSON_MODELS,
> +} cpuTestCPUIDJson;
> +
> +#if WITH_QEMU && WITH_YAJL
> +static virQEMUCapsPtr
> +cpuTestMakeQEMUCaps(const struct data *data)
> +{
> +    virQEMUCapsPtr qemuCaps = NULL;
> +    qemuMonitorTestPtr testMon = NULL;
> +    qemuMonitorCPUModelInfoPtr model = NULL;
> +    char *json = NULL;
> +
> +    if (virAsprintf(&json, "%s/cputestdata/%s-cpuid-%s.json",
> +                    abs_srcdir, virArchToString(data->arch), data->host) < 0)
> +        goto error;

could be cleanup;

> +
> +    if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true)))
> +        goto error;

same

> +
> +    if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon),
> +                                        QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC,
> +                                        "host", true, &model) < 0)
> +        goto error;

same

> +
> +    if (!(qemuCaps = virQEMUCapsNew()))
> +        goto error;

same

> +
> +    virQEMUCapsSet(qemuCaps, QEMU_CAPS_KVM);
> +    if (data->flags == JSON_MODELS)
> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_QUERY_CPU_DEFINITIONS);
> +
> +    virQEMUCapsSetArch(qemuCaps, data->arch);
> +    virQEMUCapsSetCPUModelInfo(qemuCaps, VIR_DOMAIN_VIRT_KVM, model);
> +    model = NULL;
> +
> +    if (virQEMUCapsProbeQMPCPUDefinitions(qemuCaps,
> +                                          qemuMonitorTestGetMonitor(testMon),
> +                                          false) < 0)
> +        goto error;

Leaving only this one...

> +
> + cleanup:
> +    qemuMonitorCPUModelInfoFree(model);
> +    qemuMonitorTestFree(testMon);
> +    VIR_FREE(json);
> +
> +    return qemuCaps;
> +
> + error:
> +    virObjectUnref(qemuCaps);
> +    qemuCaps = NULL;
> +    goto cleanup;
> +}
> +#endif
> +
> +
>  static int
>  cpuTestCPUID(bool guest, const void *arg)
>  {
> @@ -670,52 +728,20 @@ cpuTestUpdateLive(const void *arg)
>  }
>  
>  
> -typedef enum {
> -    JSON_NONE,
> -    JSON_HOST,
> -    JSON_MODELS,
> -} cpuTestCPUIDJson;
> -
>  #if WITH_QEMU && WITH_YAJL
>  static int
>  cpuTestJSONCPUID(const void *arg)
>  {
>      const struct data *data = arg;
> -    qemuMonitorCPUModelInfoPtr model = NULL;
>      virQEMUCapsPtr qemuCaps = NULL;
>      virCPUDefPtr cpu = NULL;
> -    qemuMonitorTestPtr testMon = NULL;
> -    char *json = NULL;
>      char *result = NULL;
>      int ret = -1;
>  
> -    if (virAsprintf(&json, "%s/cputestdata/%s-cpuid-%s.json",
> -                    abs_srcdir, virArchToString(data->arch), data->host) < 0 ||
> -        virAsprintf(&result, "cpuid-%s-json", data->host) < 0)
> +    if (virAsprintf(&result, "cpuid-%s-json", data->host) < 0)
>          goto cleanup;

This one could seemingly just return -1 although what's here works, just
a few extra steps to functions that do nothing.

With or without adjustments...

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

>  
> -    if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true)))
> -        goto cleanup;
> -
> -    if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon),
> -                                        QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC,
> -                                        "host", true, &model) < 0)
> -        goto cleanup;
> -
> -    if (!(qemuCaps = virQEMUCapsNew()))
> -        goto cleanup;
> -
> -    virQEMUCapsSet(qemuCaps, QEMU_CAPS_KVM);
> -    if (data->flags == JSON_MODELS)
> -        virQEMUCapsSet(qemuCaps, QEMU_CAPS_QUERY_CPU_DEFINITIONS);
> -
> -    virQEMUCapsSetArch(qemuCaps, data->arch);
> -    virQEMUCapsSetCPUModelInfo(qemuCaps, VIR_DOMAIN_VIRT_KVM, model);
> -    model = NULL;
> -
> -    if (virQEMUCapsProbeQMPCPUDefinitions(qemuCaps,
> -                                          qemuMonitorTestGetMonitor(testMon),
> -                                          false) < 0)
> +    if (!(qemuCaps = cpuTestMakeQEMUCaps(data)))
>          goto cleanup;
>  
>      if (VIR_ALLOC(cpu) < 0)
> @@ -732,12 +758,9 @@ cpuTestJSONCPUID(const void *arg)
>      ret = cpuTestCompareXML(data->arch, cpu, result);
>  
>   cleanup:
> -    qemuMonitorCPUModelInfoFree(model);
>      virObjectUnref(qemuCaps);
> -    qemuMonitorTestFree(testMon);
>      virCPUDefFree(cpu);
>      VIR_FREE(result);
> -    VIR_FREE(json);
>      return ret;
>  }
>  #endif
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 14/23] cputest: Separate QEMUCaps creation from cpuTestCPUIDJson
Posted by Jiri Denemark 7 years, 7 months ago
On Thu, Oct 12, 2017 at 17:20:06 -0400, John Ferlan wrote:
> 
> 
> On 10/04/2017 10:58 AM, Jiri Denemark wrote:
> > To make the code reusable by other tests.
> > 
> > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > ---
> >  tests/cputest.c | 97 +++++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 60 insertions(+), 37 deletions(-)
> > 
> > diff --git a/tests/cputest.c b/tests/cputest.c
> > index 0a07a2da14..b72c17a168 100644
> > --- a/tests/cputest.c
> > +++ b/tests/cputest.c
> > @@ -460,6 +460,64 @@ cpuTestHasFeature(const void *arg)
> >  }
> >  
> >  
> > +typedef enum {
> > +    JSON_NONE,
> > +    JSON_HOST,
> > +    JSON_MODELS,
> > +} cpuTestCPUIDJson;
> > +
> > +#if WITH_QEMU && WITH_YAJL
> > +static virQEMUCapsPtr
> > +cpuTestMakeQEMUCaps(const struct data *data)
> > +{
> > +    virQEMUCapsPtr qemuCaps = NULL;
> > +    qemuMonitorTestPtr testMon = NULL;
> > +    qemuMonitorCPUModelInfoPtr model = NULL;
> > +    char *json = NULL;
> > +
> > +    if (virAsprintf(&json, "%s/cputestdata/%s-cpuid-%s.json",
> > +                    abs_srcdir, virArchToString(data->arch), data->host) < 0)
> > +        goto error;
> 
> could be cleanup;

Yeah, but since we have the "error" label and we are jumping in case of
error I think it's slightly better to keep it as is. And it's less
fragile when someone comes and shuffles the code around. I don't really
mind either way, though.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 14/23] cputest: Separate QEMUCaps creation from cpuTestCPUIDJson
Posted by John Ferlan 7 years, 7 months ago

On 10/13/2017 01:27 PM, Jiri Denemark wrote:
> On Thu, Oct 12, 2017 at 17:20:06 -0400, John Ferlan wrote:
>>
>>
>> On 10/04/2017 10:58 AM, Jiri Denemark wrote:
>>> To make the code reusable by other tests.
>>>
>>> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
>>> ---
>>>  tests/cputest.c | 97 +++++++++++++++++++++++++++++++++++----------------------
>>>  1 file changed, 60 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/tests/cputest.c b/tests/cputest.c
>>> index 0a07a2da14..b72c17a168 100644
>>> --- a/tests/cputest.c
>>> +++ b/tests/cputest.c
>>> @@ -460,6 +460,64 @@ cpuTestHasFeature(const void *arg)
>>>  }
>>>  
>>>  
>>> +typedef enum {
>>> +    JSON_NONE,
>>> +    JSON_HOST,
>>> +    JSON_MODELS,
>>> +} cpuTestCPUIDJson;
>>> +
>>> +#if WITH_QEMU && WITH_YAJL
>>> +static virQEMUCapsPtr
>>> +cpuTestMakeQEMUCaps(const struct data *data)
>>> +{
>>> +    virQEMUCapsPtr qemuCaps = NULL;
>>> +    qemuMonitorTestPtr testMon = NULL;
>>> +    qemuMonitorCPUModelInfoPtr model = NULL;
>>> +    char *json = NULL;
>>> +
>>> +    if (virAsprintf(&json, "%s/cputestdata/%s-cpuid-%s.json",
>>> +                    abs_srcdir, virArchToString(data->arch), data->host) < 0)
>>> +        goto error;
>>
>> could be cleanup;
> 
> Yeah, but since we have the "error" label and we are jumping in case of
> error I think it's slightly better to keep it as is. And it's less
> fragile when someone comes and shuffles the code around. I don't really
> mind either way, though.
> 
> Jirka
> 

Fair enough - leave it it as is. It wasn't that important.

John

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