1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] tests: Add DO_TEST_PARSE_ERROR() to qemuxml2xml

Andrea Bolognani posted 1 patch 7 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1499174031-15411-1-git-send-email-abologna@redhat.com
tests/qemuxml2xmltest.c | 104 +++++++++++++++++++++++++++++++-----------------
1 file changed, 67 insertions(+), 37 deletions(-)
[libvirt] [PATCH] tests: Add DO_TEST_PARSE_ERROR() to qemuxml2xml
Posted by Andrea Bolognani 7 years, 10 months ago 
qemuxml2argv already supports the ability to include test
cases that are known not to make it past XML parsing, and
since we want to keep qemuxml2xml in sync with it as much
as possible, we need to implement this missing feature.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 tests/qemuxml2xmltest.c | 104 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 67 insertions(+), 37 deletions(-)

diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index c74614c..66729fa 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -28,6 +28,10 @@ enum {
     WHEN_BOTH = 3,
 };
 
+typedef enum {
+    FLAG_EXPECT_PARSE_ERROR = 1 << 0,
+} virQemuXML2XMLTestFlags;
+
 struct testInfo {
     char *inName;
     char *outActiveName;
@@ -36,6 +40,8 @@ struct testInfo {
     virBitmapPtr activeVcpus;
 
     virQEMUCapsPtr qemuCaps;
+
+    virQemuXML2XMLTestFlags flags;
 };
 
 static int
@@ -55,12 +61,15 @@ static int
 testXML2XMLActive(const void *opaque)
 {
     const struct testInfo *info = opaque;
+    testCompareDomXML2XMLResult flags = TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS;
+
+    if (info->flags & FLAG_EXPECT_PARSE_ERROR)
+        flags = TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE;
 
     return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt,
                                       info->inName, info->outActiveName, true,
                                       qemuXML2XMLActivePreFormatCallback,
-                                      opaque, 0,
-                                      TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS);
+                                      opaque, 0, flags);
 }
 
 
@@ -68,11 +77,14 @@ static int
 testXML2XMLInactive(const void *opaque)
 {
     const struct testInfo *info = opaque;
+    testCompareDomXML2XMLResult flags = TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS;
+
+    if (info->flags & FLAG_EXPECT_PARSE_ERROR)
+        flags = TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE;
 
     return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->inName,
                                       info->outInactiveName, false,
-                                      NULL, opaque, 0,
-                                      TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS);
+                                      NULL, opaque, 0, flags);
 }
 
 
@@ -195,6 +207,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
                                       VIR_DOMAIN_DEF_PARSE_STATUS |
                                       VIR_DOMAIN_DEF_PARSE_ACTUAL_NET |
                                       VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES))) {
+        if (data->flags & FLAG_EXPECT_PARSE_ERROR)
+            goto ok;
         VIR_TEST_DEBUG("Failed to parse domain status XML:\n%s", source);
         goto cleanup;
     }
@@ -217,6 +231,17 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
 
     ret = 0;
 
+ ok:
+    if (data->flags & FLAG_EXPECT_PARSE_ERROR) {
+        if (ret < 0) {
+            VIR_TEST_DEBUG("Got expected error");
+            ret = 0;
+        } else {
+            VIR_TEST_DEBUG("Error expected but there wasn't any");
+            ret = -1;
+        }
+    }
+
  cleanup:
     xmlKeepBlanksDefault(keepBlanksDefault);
     xmlFreeDoc(xml);
@@ -249,7 +274,8 @@ static int
 testInfoSet(struct testInfo *info,
             const char *name,
             int when,
-            int gic)
+            int gic,
+            virQemuXML2XMLTestFlags flags)
 {
     if (!(info->qemuCaps = virQEMUCapsNew()))
         goto error;
@@ -296,6 +322,8 @@ testInfoSet(struct testInfo *info,
         }
     }
 
+    info->flags = flags;
+
     return 0;
 
  error:
@@ -335,9 +363,9 @@ mymain(void)
     /* TODO: test with format probing disabled too */
     driver.config->allowDiskFormatProbing = true;
 
-# define DO_TEST_FULL(name, when, gic, ...)                                    \
+# define DO_TEST_FULL(name, when, gic, flags, ...)                             \
     do {                                                                       \
-        if (testInfoSet(&info, name, when, gic) < 0) {                         \
+        if (testInfoSet(&info, name, when, gic, flags) < 0) {                  \
             VIR_TEST_DEBUG("Failed to generate test data for '%s'", name);     \
             return -1;                                                         \
         }                                                                      \
@@ -364,8 +392,10 @@ mymain(void)
 # define NONE QEMU_CAPS_LAST
 
 # define DO_TEST(name, ...) \
-    DO_TEST_FULL(name, WHEN_BOTH, GIC_NONE, __VA_ARGS__)
+    DO_TEST_FULL(name, WHEN_BOTH, GIC_NONE, 0, __VA_ARGS__)
 
+# define DO_TEST_PARSE_ERROR(name, ...) \
+    DO_TEST_FULL(name, WHEN_BOTH, GIC_NONE, FLAG_EXPECT_PARSE_ERROR, __VA_ARGS__)
 
 
     /* Unset or set all envvars here that are copied in qemudBuildCommandLine
@@ -493,7 +523,7 @@ mymain(void)
             QEMU_CAPS_SCSI_DISK_WWN);
     DO_TEST("disk-mirror-old", NONE);
     DO_TEST("disk-mirror", NONE);
-    DO_TEST_FULL("disk-active-commit", WHEN_ACTIVE, GIC_NONE, NONE);
+    DO_TEST_FULL("disk-active-commit", WHEN_ACTIVE, GIC_NONE, 0, NONE);
     DO_TEST("graphics-listen-network", NONE);
     DO_TEST("graphics-vnc", NONE);
     DO_TEST("graphics-vnc-websocket", NONE);
@@ -571,7 +601,7 @@ mymain(void)
     DO_TEST("channel-virtio", NONE);
     DO_TEST("channel-virtio-state", NONE);
 
-    DO_TEST_FULL("channel-unix-source-path", WHEN_INACTIVE, GIC_NONE, NONE);
+    DO_TEST_FULL("channel-unix-source-path", WHEN_INACTIVE, GIC_NONE, 0, NONE);
 
     DO_TEST("hostdev-usb-address", NONE);
     DO_TEST("hostdev-pci-address", NONE);
@@ -644,17 +674,17 @@ mymain(void)
     DO_TEST("blkdeviotune-max-length", NONE);
     DO_TEST("controller-usb-order", NONE);
 
-    DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, GIC_NONE, NONE);
-    DO_TEST_FULL("seclabel-dynamic-override", WHEN_INACTIVE, GIC_NONE, NONE);
-    DO_TEST_FULL("seclabel-dynamic-labelskip", WHEN_INACTIVE, GIC_NONE, NONE);
-    DO_TEST_FULL("seclabel-dynamic-relabel", WHEN_INACTIVE, GIC_NONE, NONE);
+    DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, GIC_NONE, 0, NONE);
+    DO_TEST_FULL("seclabel-dynamic-override", WHEN_INACTIVE, GIC_NONE, 0, NONE);
+    DO_TEST_FULL("seclabel-dynamic-labelskip", WHEN_INACTIVE, GIC_NONE, 0, NONE);
+    DO_TEST_FULL("seclabel-dynamic-relabel", WHEN_INACTIVE, GIC_NONE, 0, NONE);
     DO_TEST("seclabel-static", NONE);
-    DO_TEST_FULL("seclabel-static-labelskip", WHEN_ACTIVE, GIC_NONE, NONE);
+    DO_TEST_FULL("seclabel-static-labelskip", WHEN_ACTIVE, GIC_NONE, 0, NONE);
     DO_TEST("seclabel-none", NONE);
     DO_TEST("seclabel-dac-none", NONE);
     DO_TEST("seclabel-dynamic-none", NONE);
     DO_TEST("seclabel-device-multiple", NONE);
-    DO_TEST_FULL("seclabel-dynamic-none-relabel", WHEN_INACTIVE, GIC_NONE, NONE);
+    DO_TEST_FULL("seclabel-dynamic-none-relabel", WHEN_INACTIVE, GIC_NONE, 0, NONE);
     DO_TEST("numad-static-vcpu-no-numatune", NONE);
 
     DO_TEST("disk-scsi-lun-passthrough-sgio",
@@ -1088,27 +1118,27 @@ mymain(void)
             QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
             QEMU_CAPS_DEVICE_VIRTIO_GPU, QEMU_CAPS_BOOTINDEX);
 
-    DO_TEST_FULL("aarch64-gic-none", WHEN_BOTH, GIC_NONE, NONE);
-    DO_TEST_FULL("aarch64-gic-none-v2", WHEN_BOTH, GIC_V2, NONE);
-    DO_TEST_FULL("aarch64-gic-none-v3", WHEN_BOTH, GIC_V3, NONE);
-    DO_TEST_FULL("aarch64-gic-none-both", WHEN_BOTH, GIC_BOTH, NONE);
-    DO_TEST_FULL("aarch64-gic-none-tcg", WHEN_BOTH, GIC_BOTH, NONE);
-    DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_NONE, NONE);
-    DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V2, NONE);
-    DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V3, NONE);
-    DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_BOTH, NONE);
-    DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_NONE, NONE);
-    DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V2, NONE);
-    DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V3, NONE);
-    DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_BOTH, NONE);
-    DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_NONE, NONE);
-    DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_V2, NONE);
-    DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_V3, NONE);
-    DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_BOTH, NONE);
-    DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_NONE, NONE);
-    DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_V2, NONE);
-    DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_V3, NONE);
-    DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_BOTH, NONE);
+    DO_TEST_FULL("aarch64-gic-none", WHEN_BOTH, GIC_NONE, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-none-v2", WHEN_BOTH, GIC_V2, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-none-v3", WHEN_BOTH, GIC_V3, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-none-both", WHEN_BOTH, GIC_BOTH, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-none-tcg", WHEN_BOTH, GIC_BOTH, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_NONE, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V2, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V3, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_BOTH, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_NONE, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V2, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V3, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_BOTH, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_NONE, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_V2, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_V3, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_BOTH, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_NONE, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_V2, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_V3, 0, NONE);
+    DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_BOTH, 0, NONE);
 
     DO_TEST("memory-hotplug", NONE);
     DO_TEST("memory-hotplug-nonuma", NONE);
-- 
2.7.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Add DO_TEST_PARSE_ERROR() to qemuxml2xml
Posted by Peter Krempa 7 years, 10 months ago 
On Tue, Jul 04, 2017 at 15:13:51 +0200, Andrea Bolognani wrote:
> qemuxml2argv already supports the ability to include test
> cases that are known not to make it past XML parsing, and
> since we want to keep qemuxml2xml in sync with it as much
> as possible, we need to implement this missing feature.

I must say that I don't really see the point to have them fail twice ...

I'd understand if this would be added to the genericxml2xmltest (if it's
not there)

> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  tests/qemuxml2xmltest.c | 104 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 67 insertions(+), 37 deletions(-)
> 
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index c74614c..66729fa 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c

> @@ -217,6 +231,17 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
>  
>      ret = 0;
>  
> + ok:

This is not an okay name for a label.

> +    if (data->flags & FLAG_EXPECT_PARSE_ERROR) {
> +        if (ret < 0) {
> +            VIR_TEST_DEBUG("Got expected error");
> +            ret = 0;

The rest looks good, but the explanation for need to do this is not that
convincing, since testing the same thing twice does not really make
sense.

I'd make more sense if you plan to move the cases which are supposed to
fail here, since this is the test dealing with XML parsing.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Add DO_TEST_PARSE_ERROR() to qemuxml2xml
Posted by Andrea Bolognani 7 years, 10 months ago 
On Fri, 2017-07-07 at 12:48 +0200, Peter Krempa wrote:
> > qemuxml2argv already supports the ability to include test
> > cases that are known not to make it past
XML parsing, and
> > since we want to keep qemuxml2xml in sync with it as much
> > as possible, we need to implement this missing feature.
> 
> I must say that I
don't really see the point to have them fail twice ...
> 
> I'd understand if this would be added to the genericxml2xmltest (if it's
> not there)

We have a lot of test cases with xml2argv coverage but no
xml2xml coverage, and vice versa. The obvious solution to
that is to, in due time, make it so you only need to list
input file and capabilities once to have them tested both
for xml2argv and xml2xml, and this is a step towards that
still pretty far away goal.

> > @@ -217,6 +231,17 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
> >  
> >      ret = 0;
> >  
> > + ok:
> 
> This is not an okay name for a label.

It's clearly an "ok" name though :P

I borrowed it from testCompareXMLToArgv() in xml2argv,
but I don't have a problem changing it to something you
think is more suitable.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Add DO_TEST_PARSE_ERROR() to qemuxml2xml
Posted by Peter Krempa 7 years, 10 months ago 
On Fri, Jul 07, 2017 at 13:00:10 +0200, Andrea Bolognani wrote:
> On Fri, 2017-07-07 at 12:48 +0200, Peter Krempa wrote:
> > > qemuxml2argv already supports the ability to include test
> > > cases that are known not to make it past
> XML parsing, and
> > > since we want to keep qemuxml2xml in sync with it as much
> > > as possible, we need to implement this missing feature.
> > 
> > I must say that I
> don't really see the point to have them fail twice ...
> > 
> > I'd understand if this would be added to the genericxml2xmltest (if it's
> > not there)
> 
> We have a lot of test cases with xml2argv coverage but no
> xml2xml coverage, and vice versa. The obvious solution to

Well. If the file fails to parse, I don't see why it would make sense to
make it fail to parse twice.

The both tests in their success paths have value. Failing to parse the
same document twice does not.

> that is to, in due time, make it so you only need to list
> input file and capabilities once to have them tested both
> for xml2argv and xml2xml, and this is a step towards that
> still pretty far away goal.

This is certainly a good worthwhile goal. I just don't really see how
splitting the testing of unparsable documents into two places or
duplicating it will help your effort to simplify it.

The combined test will have to be renamed anyways and the xml2argv test
has the capability flags, so that seems as a better point to start the
rename.

> > > @@ -217,6 +231,17 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
> > >  
> > >      ret = 0;
> > >  
> > > + ok:
> > 
> > This is not an okay name for a label.
> 
> It's clearly an "ok" name though :P
> 
> I borrowed it from testCompareXMLToArgv() in xml2argv,
> but I don't have a problem changing it to something you
> think is more suitable.

I'd prefer 'done'. 'ok' seems too short. Also the test still may fail at
that point, so 'done' should not imply success (hopefully).
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Add DO_TEST_PARSE_ERROR() to qemuxml2xml
Posted by Andrea Bolognani 7 years, 10 months ago 
On Fri, 2017-07-07 at 13:15 +0200, Peter Krempa wrote:
> > that is to, in due time, make it so you only need to list
> > input file and capabilities once to have them tested both
> > for xml2argv and xml2xml, and this is a step towards that
> > still pretty far away goal.
> 
> This is certainly a good worthwhile goal. I just don't really see how
> splitting the testing of unparsable documents into two places or
> duplicating it will help your effort to simplify it.

For some reason I was convinced that having the test in both
files would make the task of unifying the list down the line
easier, which is of course complete rubbish.

So forget this patch ever existed. I'll add the test case to
genericxml2xmltest instead, as you suggested.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.