[PATCH for 7.4.0 v2 0/3] Avoid double indentation of <metadata/> element
v2 of: https://listman.redhat.com/archives/libvir-list/2021-May/msg00767.html diff to v1: - Deduplicated the code per Pavel's suggestion Michal Prívozník (3): virxml: Introduce and use virXMLFormatMetadata() virxml: Report error if virXMLFormatMetadata() fails virxml: Avoid double indentation of <metadata/> element src/conf/domain_conf.c | 23 ++---------------- src/conf/network_conf.c | 23 ++---------------- src/libvirt_private.syms | 1 + src/util/virxml.c | 51 ++++++++++++++++++++++++++++++++++++++++ src/util/virxml.h | 3 +++ 5 files changed, 59 insertions(+), 42 deletions(-) -- 2.26.3
So far, we have to places where we format <metadata/> into XMLs: domain and network. Bot places share the same code. Move it into a helper function and just call it from those places. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 23 ++------------------- src/conf/network_conf.c | 23 ++------------------- src/libvirt_private.syms | 1 + src/util/virxml.c | 43 ++++++++++++++++++++++++++++++++++++++++ src/util/virxml.h | 3 +++ 5 files changed, 51 insertions(+), 42 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4f78b7b43d..413c44ac61 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27806,27 +27806,8 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, virBufferEscapeString(buf, "<description>%s</description>\n", def->description); - if (def->metadata) { - g_autoptr(xmlBuffer) xmlbuf = NULL; - int oldIndentTreeOutput = xmlIndentTreeOutput; - - /* Indentation on output requires that we previously set - * xmlKeepBlanksDefault to 0 when parsing; also, libxml does 2 - * spaces per level of indentation of intermediate elements, - * but no leading indentation before the starting element. - * Thankfully, libxml maps what looks like globals into - * thread-local uses, so we are thread-safe. */ - xmlIndentTreeOutput = 1; - xmlbuf = virXMLBufferCreate(); - - if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, - virBufferGetIndent(buf) / 2, 1) < 0) { - xmlIndentTreeOutput = oldIndentTreeOutput; - return -1; - } - virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf)); - xmlIndentTreeOutput = oldIndentTreeOutput; - } + if (virXMLFormatMetadata(buf, def->metadata) < 0) + return -1; if (virDomainDefHasMemoryHotplug(def)) { virBufferAsprintf(buf, diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a9eadff29c..b10ff5c7a8 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2486,27 +2486,8 @@ virNetworkDefFormatBuf(virBuffer *buf, virUUIDFormat(uuid, uuidstr); virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuidstr); - if (def->metadata) { - g_autoptr(xmlBuffer) xmlbuf = NULL; - int oldIndentTreeOutput = xmlIndentTreeOutput; - - /* Indentation on output requires that we previously set - * xmlKeepBlanksDefault to 0 when parsing; also, libxml does 2 - * spaces per level of indentation of intermediate elements, - * but no leading indentation before the starting element. - * Thankfully, libxml maps what looks like globals into - * thread-local uses, so we are thread-safe. */ - xmlIndentTreeOutput = 1; - xmlbuf = virXMLBufferCreate(); - - if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, - virBufferGetIndent(buf) / 2, 1) < 0) { - xmlIndentTreeOutput = oldIndentTreeOutput; - return -1; - } - virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf)); - xmlIndentTreeOutput = oldIndentTreeOutput; - } + if (virXMLFormatMetadata(buf, def->metadata) < 0) + return -1; if (def->forward.type != VIR_NETWORK_FORWARD_NONE) { const char *dev = NULL; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e1aef5267e..37515f80ec 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3555,6 +3555,7 @@ virXMLCheckIllegalChars; virXMLExtractNamespaceXML; virXMLFormatElement; virXMLFormatElementEmpty; +virXMLFormatMetadata; virXMLNewNode; virXMLNodeContentString; virXMLNodeNameEqual; diff --git a/src/util/virxml.c b/src/util/virxml.c index 8dcece704a..062a5402f6 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1707,6 +1707,49 @@ virXMLFormatElement(virBuffer *buf, } +/** + * virXMLFormatMetadata: + * @buf: the parent buffer where the element will be placed + * @metadata: pointer to metadata node + * + * Helper to format metadata element. If @metadata is NULL then + * this function is a NOP. + * + * Returns: 0 on success, + * -1 otherwise. + */ +int +virXMLFormatMetadata(virBuffer *buf, + xmlNodePtr metadata) +{ + g_autoptr(xmlBuffer) xmlbuf = NULL; + int oldIndentTreeOutput = xmlIndentTreeOutput; + + if (!metadata) + return 0; + + /* Indentation on output requires that we previously set + * xmlKeepBlanksDefault to 0 when parsing; also, libxml does 2 + * spaces per level of indentation of intermediate elements, + * but no leading indentation before the starting element. + * Thankfully, libxml maps what looks like globals into + * thread-local uses, so we are thread-safe. */ + xmlIndentTreeOutput = 1; + xmlbuf = virXMLBufferCreate(); + + if (xmlNodeDump(xmlbuf, metadata->doc, metadata, + virBufferGetIndent(buf) / 2, 1) < 0) { + xmlIndentTreeOutput = oldIndentTreeOutput; + return -1; + } + + virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf)); + xmlIndentTreeOutput = oldIndentTreeOutput; + + return 0; +} + + void virXPathContextNodeRestore(virXPathContextNodeSave *save) { diff --git a/src/util/virxml.h b/src/util/virxml.h index ed02abd2e9..0bb0d1c118 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -333,6 +333,9 @@ virXMLFormatElementEmpty(virBuffer *buf, virBuffer *attrBuf, virBuffer *childBuf); +int +virXMLFormatMetadata(virBuffer *buf, + xmlNodePtr metadata); struct _virXPathContextNodeSave { xmlXPathContextPtr ctxt; -- 2.26.3
I guess this is more of an academic problem, because if <metadata/> content was problematic we would have caught the error during parsing. Anyway, as is this function returns -1 without any error reported. Fix it by reporting one. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virxml.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virxml.c b/src/util/virxml.c index 062a5402f6..91c6f6b02e 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1740,6 +1740,8 @@ virXMLFormatMetadata(virBuffer *buf, if (xmlNodeDump(xmlbuf, metadata->doc, metadata, virBufferGetIndent(buf) / 2, 1) < 0) { xmlIndentTreeOutput = oldIndentTreeOutput; + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to format metadata element")); return -1; } -- 2.26.3
There was a recent change in libxml2 that caused a trouble for us. To us, <metadata/> in domain or network XMLs are just opaque value where management application can store whatever data it finds fit. At XML parser/formatter level, we just make a copy of the element during parsing and then format it back. For formatting we use xmlNodeDump() which allows caller to specify level of indentation. Previously, the indentation was not applied onto the very first line, but as of v2.9.12-2-g85b1792e libxml2 is applying indentation also on the first line. This does not work well with out virBuffer because as soon as we call virBufferAsprintf() to append <metadata/> element, virBufferAsprintf() will apply another level of indentation. Instead of version checking, let's skip any indentation added by libxml2 before virBufferAsprintf() is called. Note, the problem is only when telling xmlNodeDump() to use indentation, i.e. level argument is not zero. Therefore, virXMLNodeToString() which also calls xmlNodeDump() is safe as it passes zero. Tested-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virxml.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/util/virxml.c b/src/util/virxml.c index 91c6f6b02e..4360b15486 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1723,6 +1723,7 @@ virXMLFormatMetadata(virBuffer *buf, xmlNodePtr metadata) { g_autoptr(xmlBuffer) xmlbuf = NULL; + const char *xmlbufContent = NULL; int oldIndentTreeOutput = xmlIndentTreeOutput; if (!metadata) @@ -1745,7 +1746,12 @@ virXMLFormatMetadata(virBuffer *buf, return -1; } - virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf)); + /* After libxml2-v2.9.12-2-g85b1792e even the first line is indented. + * But virBufferAsprintf() also adds indentation. Skip one of them. */ + xmlbufContent = (const char *) xmlBufferContent(xmlbuf); + virSkipSpaces(&xmlbufContent); + + virBufferAsprintf(buf, "%s\n", xmlbufContent); xmlIndentTreeOutput = oldIndentTreeOutput; return 0; -- 2.26.3
On 5/25/21 11:48 AM, Michal Privoznik wrote:
Meanwhile, this was fixed in libxml2's upstream: https://gitlab.gnome.org/GNOME/libxml2/-/commit/13ad8736d294536da4cbcd70a96b... So I guess there won't be any regression with any upstream release of libxml2. But as I pointed out in v1, some distros are known to already backport the problematic patch (Fedora and Gentoo) so the question is how likely they are to backport the fix too. I still think it's worth fixing on our side. Michal
On Tue, May 25, 2021 at 11:57:39AM +0200, Michal Prívozník wrote:
On 5/25/21 11:48 AM, Michal Privoznik wrote:
Meanwhile, this was fixed in libxml2's upstream:
https://gitlab.gnome.org/GNOME/libxml2/-/commit/13ad8736d294536da4cbcd70a96b...
So I guess there won't be any regression with any upstream release of libxml2. But as I pointed out in v1, some distros are known to already backport the problematic patch (Fedora and Gentoo) so the question is how likely they are to backport the fix too. I still think it's worth fixing on our side.
I'm not sure if it's worth fixing, on one hand libvirt tests would be broken, on the other hand running libvirt tests could detect the broken backport and help others to create bug reports to backport the fix as well. Anyway, for the code Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On 5/25/21 12:12 PM, Pavel Hrdina wrote:
On Tue, May 25, 2021 at 11:57:39AM +0200, Michal Prívozník wrote:
On 5/25/21 11:48 AM, Michal Privoznik wrote:
Meanwhile, this was fixed in libxml2's upstream:
https://gitlab.gnome.org/GNOME/libxml2/-/commit/13ad8736d294536da4cbcd70a96b...
So I guess there won't be any regression with any upstream release of libxml2. But as I pointed out in v1, some distros are known to already backport the problematic patch (Fedora and Gentoo) so the question is how likely they are to backport the fix too. I still think it's worth fixing on our side.
I'm not sure if it's worth fixing, on one hand libvirt tests would be broken, on the other hand running libvirt tests could detect the broken backport and help others to create bug reports to backport the fix as well.
I hear you but also, if we want to freeze anytime soon we should have green pipeline :-) And also, we already have precedence of working around broken libraries in our code. Remember glib and all the fun we were having?
Anyway, for the code
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Pushed, thank you. Michal
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Pavel Hrdina