[libvirt] [PATCH 0/4] Test status XML formatting and parsing
A recent bug showed that the status XML parsing is not tested. Add test cases based on existing tests in the XML-2-XML test to excercise the parser. Additionally this series fixes also a memleak of the domain device alias list. Peter Krempa (4): qemu: domain: Don't leak device alias list util: buffer: Add support for adding text blocks with indentation tests: xml2xml: Refactor the qemu xml 2 xml test test: qemuxml2xml: Test status XML formatting and parsing src/conf/domain_conf.c | 4 +- src/conf/domain_conf.h | 9 ++ src/libvirt_private.syms | 3 + src/qemu/qemu_domain.c | 1 + src/util/virbuffer.c | 38 ++++++ src/util/virbuffer.h | 1 + tests/qemuxml2xmltest.c | 309 ++++++++++++++++++++++++++++++++++++----------- tests/virbuftest.c | 50 ++++++++ 8 files changed, 345 insertions(+), 70 deletions(-) -- 2.2.2
While adding tests for status XML parsing and formatting I've noticed that the device alias list is leaked. ==763001== 81 (48 direct, 33 indirect) bytes in 1 blocks are definitely lost in loss record 414 of 514 ==763001== at 0x4C2B8F0: calloc (vg_replace_malloc.c:623) ==763001== by 0x6ACF70F: virAllocN (viralloc.c:191) ==763001== by 0x447B64: qemuDomainObjPrivateXMLParse (qemu_domain.c:727) ==763001== by 0x6B848F9: virDomainObjParseXML (domain_conf.c:15491) ==763001== by 0x6B84CAC: virDomainObjParseNode (domain_conf.c:15608) --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 655afb9..1cf1aee 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -439,6 +439,7 @@ qemuDomainObjPrivateFree(void *data) VIR_FREE(priv->origname); virCondDestroy(&priv->unplugFinished); + virStringFreeList(priv->qemuDevices); virChrdevFree(priv->devs); /* This should never be non-NULL if we get here, but just in case... */ -- 2.2.2
The current auto-indentation buffer code applies indentation only on complete strings. To allow adding a string containing newlines and having it properly indented this patch adds virBufferAddStr. --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 38 ++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 1 + tests/virbuftest.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 33222f0..0beb44f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1099,6 +1099,7 @@ virBitmapToData; virBufferAdd; virBufferAddBuffer; virBufferAddChar; +virBufferAddStr; virBufferAdjustIndent; virBufferAsprintf; virBufferCheckErrorInternal; diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 0089d1b..50d953e 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -756,3 +756,41 @@ virBufferTrim(virBufferPtr buf, const char *str, int len) buf->use -= len < 0 ? len2 : len; buf->content[buf->use] = '\0'; } + + +/** + * virBufferAddStr: + * @buf: the buffer to append to + * @str: string to append + * + * Appends @str to @buffer. Applies autoindentation on the separate lines of + * @str. + */ +void +virBufferAddStr(virBufferPtr buf, + const char *str) +{ + size_t len = 0; + const char *start = str; + + if (!buf || !str || buf->error) + return; + + while (*str) { + len++; + + if (*str == '\n') { + virBufferAdd(buf, start, len); + str++; + len = 0; + start = str; + + continue; + } + + str++; + } + + if (len > 0) + virBufferAdd(buf, start, len); +} diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h index 24e81c7..144a1ba 100644 --- a/src/util/virbuffer.h +++ b/src/util/virbuffer.h @@ -96,5 +96,6 @@ void virBufferAdjustIndent(virBufferPtr buf, int indent); int virBufferGetIndent(const virBuffer *buf, bool dynamic); void virBufferTrim(virBufferPtr buf, const char *trim, int len); +void virBufferAddStr(virBufferPtr buf, const char *str); #endif /* __VIR_BUFFER_H__ */ diff --git a/tests/virbuftest.c b/tests/virbuftest.c index f964feb..067a77e 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -310,6 +310,44 @@ static int testBufAddBuffer(const void *data ATTRIBUTE_UNUSED) return ret; } +struct testBufAddStrData { + const char *data; + const char *expect; +}; + +static int +testBufAddStr(const void *opaque ATTRIBUTE_UNUSED) +{ + const struct testBufAddStrData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *actual; + int ret = -1; + + virBufferAddLit(&buf, "<c>\n"); + virBufferAdjustIndent(&buf, 2); + virBufferAddStr(&buf, data->data); + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</c>"); + + if (!(actual = virBufferContentAndReset(&buf))) { + TEST_ERROR("buf is empty"); + goto cleanup; + } + + if (STRNEQ_NULLABLE(actual, data->expect)) { + TEST_ERROR("testBufAddStr(): Strings don't match:\n" + "Expected:\n%s\nActual:\n%s\n", + data->expect, actual); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(actual); + return ret; +} + static int mymain(void) @@ -330,6 +368,18 @@ mymain(void) DO_TEST("Trim", testBufTrim, 0); DO_TEST("AddBuffer", testBufAddBuffer, 0); +#define DO_TEST_ADD_STR(DATA, EXPECT) \ + do { \ + struct testBufAddStrData info = { DATA, EXPECT }; \ + if (virtTestRun("Buf: AddStr", testBufAddStr, &info) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST_ADD_STR("", "<c>\n</c>"); + DO_TEST_ADD_STR("<a/>", "<c>\n <a/></c>"); + DO_TEST_ADD_STR("<a/>\n", "<c>\n <a/>\n</c>"); + DO_TEST_ADD_STR("<b>\n <a/>\n</b>\n", "<c>\n <b>\n <a/>\n </b>\n</c>"); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.2.2
On Tue, Mar 24, 2015 at 03:03:21PM +0100, Peter Krempa wrote:
The current auto-indentation buffer code applies indentation only on complete strings. To allow adding a string containing newlines and
s/strings/lines/ ? :)
having it properly indented this patch adds virBufferAddStr. --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 38 ++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 1 + tests/virbuftest.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 33222f0..0beb44f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1099,6 +1099,7 @@ virBitmapToData; virBufferAdd; virBufferAddBuffer; virBufferAddChar; +virBufferAddStr; virBufferAdjustIndent; virBufferAsprintf; virBufferCheckErrorInternal; diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 0089d1b..50d953e 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -756,3 +756,41 @@ virBufferTrim(virBufferPtr buf, const char *str, int len) buf->use -= len < 0 ? len2 : len; buf->content[buf->use] = '\0'; } + + +/** + * virBufferAddStr: + * @buf: the buffer to append to + * @str: string to append + * + * Appends @str to @buffer. Applies autoindentation on the separate lines of + * @str. + */ +void +virBufferAddStr(virBufferPtr buf, + const char *str) +{ + size_t len = 0; + const char *start = str; + + if (!buf || !str || buf->error) + return; + + while (*str) { + len++; + + if (*str == '\n') { + virBufferAdd(buf, start, len); + str++; + len = 0; + start = str; + + continue; + } + + str++; + } +
strchr() might've been more readable here, but not worth changing.
On Tue, Mar 24, 2015 at 03:03:21PM +0100, Peter Krempa wrote:
The current auto-indentation buffer code applies indentation only on complete strings. To allow adding a string containing newlines and having it properly indented this patch adds virBufferAddStr. --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 38 ++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 1 + tests/virbuftest.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+)
diff --git a/tests/virbuftest.c b/tests/virbuftest.c index f964feb..067a77e 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -310,6 +310,44 @@ static int testBufAddBuffer(const void *data ATTRIBUTE_UNUSED) return ret; }
+struct testBufAddStrData { + const char *data; + const char *expect; +}; + +static int +testBufAddStr(const void *opaque ATTRIBUTE_UNUSED) +{ + const struct testBufAddStrData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *actual; + int ret = -1; + + virBufferAddLit(&buf, "<c>\n"); + virBufferAdjustIndent(&buf, 2); + virBufferAddStr(&buf, data->data); + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</c>"); + + if (!(actual = virBufferContentAndReset(&buf))) { + TEST_ERROR("buf is empty"); + goto cleanup; + } + + if (STRNEQ_NULLABLE(actual, data->expect)) { + TEST_ERROR("testBufAddStr(): Strings don't match:\n" + "Expected:\n%s\nActual:\n%s\n", + data->expect, actual);
One more question though, virtTestDifferenceFull() doens't make sense here?
On Wed, Mar 25, 2015 at 10:32:11 +0100, Martin Kletzander wrote:
On Tue, Mar 24, 2015 at 03:03:21PM +0100, Peter Krempa wrote:
The current auto-indentation buffer code applies indentation only on complete strings. To allow adding a string containing newlines and having it properly indented this patch adds virBufferAddStr. --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 38 ++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 1 + tests/virbuftest.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+)
diff --git a/tests/virbuftest.c b/tests/virbuftest.c index f964feb..067a77e 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -310,6 +310,44 @@ static int testBufAddBuffer(const void *data ATTRIBUTE_UNUSED) return ret; }
+struct testBufAddStrData { + const char *data; + const char *expect; +}; + +static int +testBufAddStr(const void *opaque ATTRIBUTE_UNUSED) +{ + const struct testBufAddStrData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *actual; + int ret = -1; + + virBufferAddLit(&buf, "<c>\n"); + virBufferAdjustIndent(&buf, 2); + virBufferAddStr(&buf, data->data); + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</c>"); + + if (!(actual = virBufferContentAndReset(&buf))) { + TEST_ERROR("buf is empty"); + goto cleanup; + } + + if (STRNEQ_NULLABLE(actual, data->expect)) { + TEST_ERROR("testBufAddStr(): Strings don't match:\n" + "Expected:\n%s\nActual:\n%s\n", + data->expect, actual);
One more question though, virtTestDifferenceFull() doens't make sense here?
Actually the output with virtTestDifferenceFull kind of sucks with short documents. The following part is with the existing code: 8) Buf: AddStr ... testBufAddStr(): Strings don't match: Expected: <c> <a/> </c> Actual: <c> <a/></c> FAILED 9) Buf: AddStr ... testBufAddStr(): Strings don't match: Expected: <c> <b> <a/> </b> </c> Actual: <c> <b> <a/></b></c> FAILED While this part is with virtTestDifference(): 8) Buf: AddStr ... testBufAddStr(): Strings don't match: Offset 0 Expect [<c> <a/> </c>] Actual [<c> <a/></c>] ... FAILED 9) Buf: AddStr ... testBufAddStr(): Strings don't match: Offset 0 Expect [<c> <b> <a/> </b> </c>] Actual [<c> <b> <a/></b></c>] ... FAILED Peter
On Wed, Mar 25, 2015 at 01:00:17PM +0100, Peter Krempa wrote:
On Wed, Mar 25, 2015 at 10:32:11 +0100, Martin Kletzander wrote:
On Tue, Mar 24, 2015 at 03:03:21PM +0100, Peter Krempa wrote:
The current auto-indentation buffer code applies indentation only on complete strings. To allow adding a string containing newlines and having it properly indented this patch adds virBufferAddStr. --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 38 ++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 1 + tests/virbuftest.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+)
diff --git a/tests/virbuftest.c b/tests/virbuftest.c index f964feb..067a77e 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -310,6 +310,44 @@ static int testBufAddBuffer(const void *data ATTRIBUTE_UNUSED) return ret; }
+struct testBufAddStrData { + const char *data; + const char *expect; +}; + +static int +testBufAddStr(const void *opaque ATTRIBUTE_UNUSED) +{ + const struct testBufAddStrData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *actual; + int ret = -1; + + virBufferAddLit(&buf, "<c>\n"); + virBufferAdjustIndent(&buf, 2); + virBufferAddStr(&buf, data->data); + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</c>"); + + if (!(actual = virBufferContentAndReset(&buf))) { + TEST_ERROR("buf is empty"); + goto cleanup; + } + + if (STRNEQ_NULLABLE(actual, data->expect)) { + TEST_ERROR("testBufAddStr(): Strings don't match:\n" + "Expected:\n%s\nActual:\n%s\n", + data->expect, actual);
One more question though, virtTestDifferenceFull() doens't make sense here?
Actually the output with virtTestDifferenceFull kind of sucks with short documents.
I find it cleaner, but that's highly subjective ;) go ahead and push it as-is, the ACK still stands.
The following part is with the existing code: 8) Buf: AddStr ... testBufAddStr(): Strings don't match: Expected: <c> <a/> </c> Actual: <c> <a/></c> FAILED 9) Buf: AddStr ... testBufAddStr(): Strings don't match: Expected: <c> <b> <a/> </b> </c> Actual: <c> <b> <a/></b></c> FAILED
While this part is with virtTestDifference(): 8) Buf: AddStr ... testBufAddStr(): Strings don't match:
Offset 0 Expect [<c> <a/> </c>] Actual [<c> <a/></c>] ... FAILED 9) Buf: AddStr ... testBufAddStr(): Strings don't match:
Offset 0 Expect [<c> <b> <a/> </b> </c>] Actual [<c> <b> <a/></b></c>] ... FAILED
Peter
On Wed, Mar 25, 2015 at 13:10:49 +0100, Martin Kletzander wrote:
On Wed, Mar 25, 2015 at 01:00:17PM +0100, Peter Krempa wrote:
On Wed, Mar 25, 2015 at 10:32:11 +0100, Martin Kletzander wrote:
On Tue, Mar 24, 2015 at 03:03:21PM +0100, Peter Krempa wrote:
The current auto-indentation buffer code applies indentation only on complete strings. To allow adding a string containing newlines and having it properly indented this patch adds virBufferAddStr. --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 38 ++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 1 + tests/virbuftest.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+)
diff --git a/tests/virbuftest.c b/tests/virbuftest.c index f964feb..067a77e 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -310,6 +310,44 @@ static int testBufAddBuffer(const void *data ATTRIBUTE_UNUSED) return ret; }
+struct testBufAddStrData { + const char *data; + const char *expect; +}; + +static int +testBufAddStr(const void *opaque ATTRIBUTE_UNUSED) +{ + const struct testBufAddStrData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *actual; + int ret = -1; + + virBufferAddLit(&buf, "<c>\n"); + virBufferAdjustIndent(&buf, 2); + virBufferAddStr(&buf, data->data); + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</c>"); + + if (!(actual = virBufferContentAndReset(&buf))) { + TEST_ERROR("buf is empty"); + goto cleanup; + } + + if (STRNEQ_NULLABLE(actual, data->expect)) { + TEST_ERROR("testBufAddStr(): Strings don't match:\n" + "Expected:\n%s\nActual:\n%s\n", + data->expect, actual);
One more question though, virtTestDifferenceFull() doens't make sense here?
Actually the output with virtTestDifferenceFull kind of sucks with short documents.
I find it cleaner, but that's highly subjective ;) go ahead and push it as-is, the ACK still stands.
I've also changed to the strchr approach as it's really better, so I'll repost with that and also virtTestDifference() since I've switched to it when writing the mail :) Peter
To allow adding more tests, refactor the XML-2-XML test so that the files are not reloaded always and clarify the control flow. Result of this changes is that the active and inactive portions of the XML are tested in separate steps rather than one test step. --- tests/qemuxml2xmltest.c | 214 +++++++++++++++++++++++++++++++----------------- 1 file changed, 140 insertions(+), 74 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0f16d5e..627edca 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -22,11 +22,30 @@ static virQEMUDriver driver; +enum { + WHEN_INACTIVE = 1, + WHEN_ACTIVE = 2, + WHEN_EITHER = 3, +}; + +struct testInfo { + char *inName; + char *inFile; + + char *outActiveName; + char *outActiveFile; + + char *outInactiveName; + char *outInactiveFile; +}; + static int -testCompareXMLToXMLFiles(const char *inxml, const char *outxml, bool live) +testXML2XMLHelper(const char *inxml, + const char *inXmlData, + const char *outxml, + const char *outXmlData, + bool live) { - char *inXmlData = NULL; - char *outXmlData = NULL; char *actual = NULL; int ret = -1; virDomainDefPtr def = NULL; @@ -35,11 +54,6 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml, bool live) if (!live) format_flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; - if (virtTestLoadFile(inxml, &inXmlData) < 0) - goto fail; - if (virtTestLoadFile(outxml, &outXmlData) < 0) - goto fail; - if (!(def = virDomainDefParseString(inXmlData, driver.caps, driver.xmlopt, QEMU_EXPECTED_VIRT_TYPES, parse_flags))) goto fail; @@ -58,82 +72,120 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml, bool live) } ret = 0; + fail: - VIR_FREE(inXmlData); - VIR_FREE(outXmlData); VIR_FREE(actual); virDomainDefFree(def); return ret; } -enum { - WHEN_INACTIVE = 1, - WHEN_ACTIVE = 2, - WHEN_EITHER = 3, -}; -struct testInfo { - const char *name; - bool different; - int when; -}; +static int +testXML2XMLActive(const void *opaque) +{ + const struct testInfo *info = opaque; + + return testXML2XMLHelper(info->inName, + info->inFile, + info->outActiveName, + info->outActiveFile, + true); +} + static int -testCompareXMLToXMLHelper(const void *data) +testXML2XMLInactive(const void *opaque) { - const struct testInfo *info = data; - char *xml_in = NULL; - char *xml_out = NULL; - char *xml_out_active = NULL; - char *xml_out_inactive = NULL; - int ret = -1; + const struct testInfo *info = opaque; + + return testXML2XMLHelper(info->inName, + info->inFile, + info->outInactiveName, + info->outInactiveFile, + false); +} + - if (virAsprintf(&xml_in, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml", - abs_srcdir, info->name) < 0 || - virAsprintf(&xml_out, "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s.xml", - abs_srcdir, info->name) < 0 || - virAsprintf(&xml_out_active, - "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s-active.xml", - abs_srcdir, info->name) < 0 || - virAsprintf(&xml_out_inactive, - "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s-inactive.xml", - abs_srcdir, info->name) < 0) - goto cleanup; - - if ((info->when & WHEN_INACTIVE)) { - char *out; - if (!info->different) - out = xml_in; - else if (virFileExists(xml_out_inactive)) - out = xml_out_inactive; - else - out = xml_out; - - if (testCompareXMLToXMLFiles(xml_in, out, false) < 0) - goto cleanup; +static void +testInfoFree(struct testInfo *info) +{ + VIR_FREE(info->inName); + VIR_FREE(info->inFile); + + VIR_FREE(info->outActiveName); + VIR_FREE(info->outActiveFile); + + VIR_FREE(info->outInactiveName); + VIR_FREE(info->outInactiveFile); +} + + +static int +testInfoSet(struct testInfo *info, + const char *name, + bool different, + int when) +{ + if (virAsprintf(&info->inName, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml", + abs_srcdir, name) < 0) + goto error; + + if (virtTestLoadFile(info->inName, &info->inFile) < 0) + goto error; + + if (when & WHEN_INACTIVE) { + if (different) { + if (virAsprintf(&info->outInactiveName, + "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s-inactive.xml", + abs_srcdir, name) < 0) + goto error; + + if (!virFileExists(info->outInactiveName)) { + VIR_FREE(info->outInactiveName); + + if (virAsprintf(&info->outInactiveName, + "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s.xml", + abs_srcdir, name) < 0) + goto error; + } + } else { + if (VIR_STRDUP(info->outInactiveName, info->inName) < 0) + goto error; + } + + if (virtTestLoadFile(info->outInactiveName, &info->outInactiveFile) < 0) + goto error; } - if ((info->when & WHEN_ACTIVE)) { - char *out; - if (!info->different) - out = xml_in; - else if (virFileExists(xml_out_active)) - out = xml_out_active; - else - out = xml_out; - - if (testCompareXMLToXMLFiles(xml_in, out, true) < 0) - goto cleanup; + if (when & WHEN_ACTIVE) { + if (different) { + if (virAsprintf(&info->outActiveName, + "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s-active.xml", + abs_srcdir, name) < 0) + goto error; + + if (!virFileExists(info->outActiveName)) { + VIR_FREE(info->outActiveName); + + if (virAsprintf(&info->outActiveName, + "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s.xml", + abs_srcdir, name) < 0) + goto error; + } + } else { + if (VIR_STRDUP(info->outActiveName, info->inName) < 0) + goto error; + } + + if (virtTestLoadFile(info->outActiveName, &info->outActiveFile) < 0) + goto error; } - ret = 0; + return 0; - cleanup: - VIR_FREE(xml_in); - VIR_FREE(xml_out); - VIR_FREE(xml_out_active); - VIR_FREE(xml_out_inactive); - return ret; + error: + testInfoFree(info); + return -1; } @@ -141,6 +193,7 @@ static int mymain(void) { int ret = 0; + struct testInfo info; if ((driver.caps = testQemuCapsInit()) == NULL) return EXIT_FAILURE; @@ -148,12 +201,25 @@ mymain(void) if (!(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver))) return EXIT_FAILURE; -# define DO_TEST_FULL(name, is_different, when) \ - do { \ - const struct testInfo info = {name, is_different, when}; \ - if (virtTestRun("QEMU XML-2-XML " name, \ - testCompareXMLToXMLHelper, &info) < 0) \ - ret = -1; \ +# define DO_TEST_FULL(name, is_different, when) \ + do { \ + if (testInfoSet(&info, name, is_different, when) < 0) { \ + fprintf(stderr, "Failed to generate test data for '%s'", name); \ + return -1; \ + } \ + \ + if (info.outInactiveName) { \ + if (virtTestRun("QEMU XML-2-XML-inactive " name, \ + testXML2XMLInactive, &info) < 0) \ + ret = -1; \ + } \ + \ + if (info.outActiveName) { \ + if (virtTestRun("QEMU XML-2-XML-active " name, \ + testXML2XMLActive, &info) < 0) \ + ret = -1; \ + } \ + testInfoFree(&info); \ } while (0) # define DO_TEST(name) \ -- 2.2.2
On Tue, Mar 24, 2015 at 03:03:22PM +0100, Peter Krempa wrote:
To allow adding more tests, refactor the XML-2-XML test so that the files are not reloaded always and clarify the control flow.
Result of this changes is that the active and inactive portions of the XML are tested in separate steps rather than one test step. --- tests/qemuxml2xmltest.c | 214 +++++++++++++++++++++++++++++++----------------- 1 file changed, 140 insertions(+), 74 deletions(-)
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0f16d5e..627edca 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -22,11 +22,30 @@
static virQEMUDriver driver;
+enum { + WHEN_INACTIVE = 1, + WHEN_ACTIVE = 2, + WHEN_EITHER = 3,
Pre-existing, but reading the code, I guess this "either" means "both", doesn't it?
@@ -148,12 +201,25 @@ mymain(void) if (!(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver))) return EXIT_FAILURE;
-# define DO_TEST_FULL(name, is_different, when) \ - do { \ - const struct testInfo info = {name, is_different, when}; \ - if (virtTestRun("QEMU XML-2-XML " name, \ - testCompareXMLToXMLHelper, &info) < 0) \ - ret = -1; \ +# define DO_TEST_FULL(name, is_different, when) \ + do { \ + if (testInfoSet(&info, name, is_different, when) < 0) { \ + fprintf(stderr, "Failed to generate test data for '%s'", name); \ + return -1; \ + } \ + \ + if (info.outInactiveName) { \ + if (virtTestRun("QEMU XML-2-XML-inactive " name, \ + testXML2XMLInactive, &info) < 0) \ + ret = -1; \ + } \ + \ + if (info.outActiveName) { \ + if (virtTestRun("QEMU XML-2-XML-active " name, \ + testXML2XMLActive, &info) < 0) \ + ret = -1; \ + } \ + testInfoFree(&info); \
s/ // (indentation's off)
On Wed, Mar 25, 2015 at 10:13:44 +0100, Martin Kletzander wrote:
On Tue, Mar 24, 2015 at 03:03:22PM +0100, Peter Krempa wrote:
To allow adding more tests, refactor the XML-2-XML test so that the files are not reloaded always and clarify the control flow.
Result of this changes is that the active and inactive portions of the XML are tested in separate steps rather than one test step. --- tests/qemuxml2xmltest.c | 214 +++++++++++++++++++++++++++++++----------------- 1 file changed, 140 insertions(+), 74 deletions(-)
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0f16d5e..627edca 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -22,11 +22,30 @@
static virQEMUDriver driver;
+enum { + WHEN_INACTIVE = 1, + WHEN_ACTIVE = 2, + WHEN_EITHER = 3,
Pre-existing, but reading the code, I guess this "either" means "both", doesn't it?
I changed this to _BOTH
@@ -148,12 +201,25 @@ mymain(void) if (!(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver))) return EXIT_FAILURE;
-# define DO_TEST_FULL(name, is_different, when) \ - do { \ - const struct testInfo info = {name, is_different, when}; \ - if (virtTestRun("QEMU XML-2-XML " name, \ - testCompareXMLToXMLHelper, &info) < 0) \ - ret = -1; \ +# define DO_TEST_FULL(name, is_different, when) \ + do { \ + if (testInfoSet(&info, name, is_different, when) < 0) { \ + fprintf(stderr, "Failed to generate test data for '%s'", name); \ + return -1; \ + } \ + \ + if (info.outInactiveName) { \ + if (virtTestRun("QEMU XML-2-XML-inactive " name, \ + testXML2XMLInactive, &info) < 0) \ + ret = -1; \ + } \ + \ + if (info.outActiveName) { \ + if (virtTestRun("QEMU XML-2-XML-active " name, \ + testXML2XMLActive, &info) < 0) \ + ret = -1; \ + } \ + testInfoFree(&info); \
s/ // (indentation's off)
And fixed this problem. And pushed this patch and 1/4. I'll repost the rest as I've done pretty serious changes to 2/4.
Recently we've fixed a bug where the status XML could not be parsed as the parser used absolute path XPath queries. This test enhancement tests all XML files used in the qemu-xml-2-xml test as a part of a status XML snippet to see whether they are parsed correctly. The status XML-2-XML is currently tested in 223 cases with this patch. --- src/conf/domain_conf.c | 4 +- src/conf/domain_conf.h | 9 ++++ src/libvirt_private.syms | 2 + tests/qemuxml2xmltest.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d633f93..d28b62a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15580,7 +15580,7 @@ virDomainDefParseNode(xmlDocPtr xml, } -static virDomainObjPtr +virDomainObjPtr virDomainObjParseNode(xmlDocPtr xml, xmlNodePtr root, virCapsPtr caps, @@ -21252,7 +21252,7 @@ virDomainDefFormat(virDomainDefPtr def, unsigned int flags) } -static char * +char * virDomainObjFormat(virDomainXMLOptionPtr xmlopt, virDomainObjPtr obj, unsigned int flags) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bceb2d7..608f61f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2565,6 +2565,12 @@ virDomainDefPtr virDomainDefParseNode(xmlDocPtr doc, virDomainXMLOptionPtr xmlopt, unsigned int expectedVirtTypes, unsigned int flags); +virDomainObjPtr virDomainObjParseNode(xmlDocPtr xml, + xmlNodePtr root, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int expectedVirtTypes, + unsigned int flags); virDomainObjPtr virDomainObjParseFile(const char *filename, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, @@ -2580,6 +2586,9 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); char *virDomainDefFormat(virDomainDefPtr def, unsigned int flags); +char *virDomainObjFormat(virDomainXMLOptionPtr xmlopt, + virDomainObjPtr obj, + unsigned int flags); int virDomainDefFormatInternal(virDomainDefPtr def, unsigned int flags, virBufferPtr buf); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0beb44f..4ab8638 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -364,6 +364,7 @@ virDomainNostateReasonTypeFromString; virDomainNostateReasonTypeToString; virDomainObjAssignDef; virDomainObjCopyPersistentDef; +virDomainObjFormat; virDomainObjGetMetadata; virDomainObjGetPersistentDef; virDomainObjGetState; @@ -382,6 +383,7 @@ virDomainObjListNumOfDomains; virDomainObjListRemove; virDomainObjListRemoveLocked; virDomainObjNew; +virDomainObjParseNode; virDomainObjSetDefTransient; virDomainObjSetMetadata; virDomainObjSetState; diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 627edca..b419231 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -106,6 +106,109 @@ testXML2XMLInactive(const void *opaque) } +static const char testStatusXMLPrefix[] = +"<domstatus state='running' reason='booted' pid='3803518'>\n" +" <taint flag='high-privileges'/>\n" +" <monitor path='/var/lib/libvirt/qemu/test.monitor' json='1' type='unix'/>\n" +" <vcpus>\n" +" <vcpu pid='3803519'/>\n" +" </vcpus>\n" +" <qemuCaps>\n" +" <flag name='vnc-colon'/>\n" +" <flag name='no-reboot'/>\n" +" <flag name='drive'/>\n" +" <flag name='name'/>\n" +" <flag name='uuid'/>\n" +" <flag name='vnet-hdr'/>\n" +" <flag name='qxl.vgamem_mb'/>\n" +" <flag name='qxl-vga.vgamem_mb'/>\n" +" <flag name='pc-dimm'/>\n" +" </qemuCaps>\n" +" <devices>\n" +" <device alias='balloon0'/>\n" +" <device alias='video0'/>\n" +" <device alias='serial0'/>\n" +" <device alias='net0'/>\n" +" <device alias='usb'/>\n" +" </devices>\n"; + +static const char testStatusXMLSuffix[] = +"</domstatus>\n"; + + +static int +testCompareStatusXMLToXMLFiles(const void *opaque) +{ + const struct testInfo *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; + xmlDocPtr xml = NULL; + virDomainObjPtr obj = NULL; + char *expect = NULL; + char *actual = NULL; + char *source = NULL; + int ret = -1; + int keepBlanksDefault = xmlKeepBlanksDefault(0); + + /* construct faked source status XML */ + virBufferAdd(&buf, testStatusXMLPrefix, -1); + virBufferAdjustIndent(&buf, 2); + virBufferAddStr(&buf, data->inFile); + virBufferAdjustIndent(&buf, -2); + virBufferAdd(&buf, testStatusXMLSuffix, -1); + + if (!(source = virBufferContentAndReset(&buf))) { + fprintf(stderr, "Failed to create the source XML"); + goto cleanup; + } + + /* construct the expect string */ + virBufferAdd(&buf, testStatusXMLPrefix, -1); + virBufferAdjustIndent(&buf, 2); + virBufferAddStr(&buf, data->outActiveFile); + virBufferAdjustIndent(&buf, -2); + virBufferAdd(&buf, testStatusXMLSuffix, -1); + + if (!(expect = virBufferContentAndReset(&buf))) { + fprintf(stderr, "Failed to create the expect XML"); + goto cleanup; + } + + /* parse the fake source status XML */ + if (!(xml = virXMLParseString(source, "(domain_status_test_XML)")) || + !(obj = virDomainObjParseNode(xml, xmlDocGetRootElement(xml), + driver.caps, driver.xmlopt, + QEMU_EXPECTED_VIRT_TYPES, 0))) { + fprintf(stderr, "Failed to parse domain status XML:\n%s", source); + goto cleanup; + } + + /* format it back */ + if (!(actual = virDomainObjFormat(driver.xmlopt, obj, + VIR_DOMAIN_DEF_FORMAT_SECURE))) { + fprintf(stderr, "Failed to format domain status XML"); + goto cleanup; + } + + if (STRNEQ(actual, expect)) { + virtTestDifferenceFull(stderr, + expect, data->outActiveName, + actual, data->inName); + goto cleanup; + } + + ret = 0; + + cleanup: + xmlKeepBlanksDefault(keepBlanksDefault); + xmlFreeDoc(xml); + virObjectUnref(obj); + VIR_FREE(expect); + VIR_FREE(actual); + VIR_FREE(source); + return ret; +} + + static void testInfoFree(struct testInfo *info) { @@ -218,6 +321,10 @@ mymain(void) if (virtTestRun("QEMU XML-2-XML-active " name, \ testXML2XMLActive, &info) < 0) \ ret = -1; \ + \ + if (virtTestRun("QEMU status XML-2-XML " name, \ + testCompareStatusXMLToXMLFiles, &info) < 0) \ + ret = -1; \ } \ testInfoFree(&info); \ } while (0) -- 2.2.2
On Tue, Mar 24, 2015 at 03:03:23PM +0100, Peter Krempa wrote:
Recently we've fixed a bug where the status XML could not be parsed as the parser used absolute path XPath queries. This test enhancement tests all XML files used in the qemu-xml-2-xml test as a part of a status XML snippet to see whether they are parsed correctly. The status XML-2-XML is currently tested in 223 cases with this patch. --- src/conf/domain_conf.c | 4 +- src/conf/domain_conf.h | 9 ++++ src/libvirt_private.syms | 2 + tests/qemuxml2xmltest.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d633f93..d28b62a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15580,7 +15580,7 @@ virDomainDefParseNode(xmlDocPtr xml, }
-static virDomainObjPtr +virDomainObjPtr virDomainObjParseNode(xmlDocPtr xml, xmlNodePtr root, virCapsPtr caps, @@ -21252,7 +21252,7 @@ virDomainDefFormat(virDomainDefPtr def, unsigned int flags) }
-static char * +char * virDomainObjFormat(virDomainXMLOptionPtr xmlopt, virDomainObjPtr obj, unsigned int flags) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bceb2d7..608f61f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2565,6 +2565,12 @@ virDomainDefPtr virDomainDefParseNode(xmlDocPtr doc, virDomainXMLOptionPtr xmlopt, unsigned int expectedVirtTypes, unsigned int flags); +virDomainObjPtr virDomainObjParseNode(xmlDocPtr xml, + xmlNodePtr root, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int expectedVirtTypes, + unsigned int flags); virDomainObjPtr virDomainObjParseFile(const char *filename, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, @@ -2580,6 +2586,9 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags);
char *virDomainDefFormat(virDomainDefPtr def, unsigned int flags); +char *virDomainObjFormat(virDomainXMLOptionPtr xmlopt, + virDomainObjPtr obj, + unsigned int flags); int virDomainDefFormatInternal(virDomainDefPtr def, unsigned int flags, virBufferPtr buf); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0beb44f..4ab8638 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -364,6 +364,7 @@ virDomainNostateReasonTypeFromString; virDomainNostateReasonTypeToString; virDomainObjAssignDef; virDomainObjCopyPersistentDef; +virDomainObjFormat; virDomainObjGetMetadata; virDomainObjGetPersistentDef; virDomainObjGetState; @@ -382,6 +383,7 @@ virDomainObjListNumOfDomains; virDomainObjListRemove; virDomainObjListRemoveLocked; virDomainObjNew; +virDomainObjParseNode; virDomainObjSetDefTransient; virDomainObjSetMetadata; virDomainObjSetState; diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 627edca..b419231 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -106,6 +106,109 @@ testXML2XMLInactive(const void *opaque) }
+static const char testStatusXMLPrefix[] = +"<domstatus state='running' reason='booted' pid='3803518'>\n" +" <taint flag='high-privileges'/>\n" +" <monitor path='/var/lib/libvirt/qemu/test.monitor' json='1' type='unix'/>\n" +" <vcpus>\n" +" <vcpu pid='3803519'/>\n" +" </vcpus>\n" +" <qemuCaps>\n" +" <flag name='vnc-colon'/>\n" +" <flag name='no-reboot'/>\n" +" <flag name='drive'/>\n" +" <flag name='name'/>\n" +" <flag name='uuid'/>\n" +" <flag name='vnet-hdr'/>\n" +" <flag name='qxl.vgamem_mb'/>\n" +" <flag name='qxl-vga.vgamem_mb'/>\n" +" <flag name='pc-dimm'/>\n" +" </qemuCaps>\n" +" <devices>\n" +" <device alias='balloon0'/>\n" +" <device alias='video0'/>\n" +" <device alias='serial0'/>\n" +" <device alias='net0'/>\n" +" <device alias='usb'/>\n" +" </devices>\n"; + +static const char testStatusXMLSuffix[] = +"</domstatus>\n"; + + +static int +testCompareStatusXMLToXMLFiles(const void *opaque) +{ + const struct testInfo *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; + xmlDocPtr xml = NULL; + virDomainObjPtr obj = NULL; + char *expect = NULL; + char *actual = NULL; + char *source = NULL; + int ret = -1; + int keepBlanksDefault = xmlKeepBlanksDefault(0); + + /* construct faked source status XML */ + virBufferAdd(&buf, testStatusXMLPrefix, -1); + virBufferAdjustIndent(&buf, 2); + virBufferAddStr(&buf, data->inFile); + virBufferAdjustIndent(&buf, -2); + virBufferAdd(&buf, testStatusXMLSuffix, -1); + + if (!(source = virBufferContentAndReset(&buf))) { + fprintf(stderr, "Failed to create the source XML"); + goto cleanup; + } + + /* construct the expect string */ + virBufferAdd(&buf, testStatusXMLPrefix, -1); + virBufferAdjustIndent(&buf, 2); + virBufferAddStr(&buf, data->outActiveFile); + virBufferAdjustIndent(&buf, -2); + virBufferAdd(&buf, testStatusXMLSuffix, -1); + + if (!(expect = virBufferContentAndReset(&buf))) { + fprintf(stderr, "Failed to create the expect XML"); + goto cleanup; + } + + /* parse the fake source status XML */ + if (!(xml = virXMLParseString(source, "(domain_status_test_XML)")) || + !(obj = virDomainObjParseNode(xml, xmlDocGetRootElement(xml), + driver.caps, driver.xmlopt, + QEMU_EXPECTED_VIRT_TYPES, 0))) {
Looking at how parsing of status XMLs work in QEMU, there are some flags passed here. Particularly: VIR_DOMAIN_DEF_PARSE_STATUS | VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | VIR_DOMAIN_DEF_PARSE_CLOCK_ADJUST Some of these are not used at all anywhere in the code (CLOCK_ADJUST), some will not affect the XML parsing of any XML we test (PCI_ORIG_STATES | ACTUAL_NET). And even with all of them (with PARSE_STATUS as well) added to the flags, none of the tests fail. I suspect this is because we currently test no status XMLs in this test. But *if* we are going to, shouldn't the following diff be squashed in? diff --git i/tests/qemuxml2xmltest.c w/tests/qemuxml2xmltest.c index b419231..1566c8a 100644 --- i/tests/qemuxml2xmltest.c +++ w/tests/qemuxml2xmltest.c @@ -177,7 +177,11 @@ testCompareStatusXMLToXMLFiles(const void *opaque) if (!(xml = virXMLParseString(source, "(domain_status_test_XML)")) || !(obj = virDomainObjParseNode(xml, xmlDocGetRootElement(xml), driver.caps, driver.xmlopt, - QEMU_EXPECTED_VIRT_TYPES, 0))) { + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_DEF_PARSE_STATUS | + VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | + VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | + VIR_DOMAIN_DEF_PARSE_CLOCK_ADJUST))) { fprintf(stderr, "Failed to parse domain status XML:\n%s", source); goto cleanup; } --
@@ -218,6 +321,10 @@ mymain(void) if (virtTestRun("QEMU XML-2-XML-active " name, \ testXML2XMLActive, &info) < 0) \ ret = -1; \ + \ + if (virtTestRun("QEMU status XML-2-XML " name, \
QEMU XML-2-XML-status would look like other cases.
On Tue, Mar 24, 2015 at 03:03:19PM +0100, Peter Krempa wrote:
A recent bug showed that the status XML parsing is not tested. Add test cases based on existing tests in the XML-2-XML test to excercise the parser.
Additionally this series fixes also a memleak of the domain device alias list.
Peter Krempa (4): qemu: domain: Don't leak device alias list util: buffer: Add support for adding text blocks with indentation tests: xml2xml: Refactor the qemu xml 2 xml test test: qemuxml2xml: Test status XML formatting and parsing
<nit-picking> What's the difference between "tests: xml2xml:" and "test: qemuxml2xml:"? :-) </nit-picking> ACK to 1/4 -- 3/4, but see comments inline. Question in 4/4 in a minute.
participants (2)
-
Martin Kletzander -
Peter Krempa