Re: [PATCH for 7.4.0] conf: Avoid double indentation of <metadata/> element

On 5/25/21 10:47 AM, Daniel P. Berrangé wrote: > On Tue, May 25, 2021 at 09:32:35AM +0200, Michal Privoznik wrote: >> 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. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> >> I've raised this issue with libxml2 team: >> >> https://gitlab.gnome.org/GNOME/libxml2/-/commit/85b1792e37b131e7a51af98a3... >> >> but I'm not sure it'll be fixed. Unfortunately, the patch that's causing >> us trouble is being backported all over the place, because it's >> supposedly fixing a regression. >> >> src/conf/domain_conf.c | 9 ++++++++- >> src/conf/network_conf.c | 9 ++++++++- >> 2 files changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 4f78b7b43d..84a8c269be 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -27808,6 +27808,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, >> >> if (def->metadata) { >> g_autoptr(xmlBuffer) xmlbuf = NULL; >> + const char *xmlbufContent = NULL; >> int oldIndentTreeOutput = xmlIndentTreeOutput; >> >> /* Indentation on output requires that we previously set >> @@ -27824,7 +27825,13 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, >> xmlIndentTreeOutput = oldIndentTreeOutput; >> 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); > > Won't this mean that indent is now wrong for the older libxml ? No, because the older did not put any spaces at the beginning. I mean, with older libxml we get: xmlbufContent = "<metadata>\n blah blah ..."; with new new libxl we get: xmlbufContent = " <metadata>\n blah blah ..."; Hence virSkipSpaces() which drops those leading spaces.