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

[Daniel P. Berrangé]

On Tue, May 25, 2021 at 10:52:49AM +0200, Michal Prívozník wrote:
> 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 at redhat.com>
> >> ---
> >>
> >> I've raised this issue with libxml2 team:
> >>
> >> https://gitlab.gnome.org/GNOME/libxml2/-/commit/85b1792e37b131e7a51af98a37f92472e8de5f3f
> >>
> >> 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.

Ah, you're stripping the spaces that libxml added, rather than the
spaces that virBuffer added.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|