1. Patchew
  2. Libvirt
  3. View series

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

Michal Privoznik posted 1 patch 2 years, 11 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/ed352a4b0d77a295aa64ade6dbe7b085feb2c280.1621927779.git.mprivozn@redhat.com
src/conf/domain_conf.c  | 9 ++++++++-
src/conf/network_conf.c | 9 ++++++++-
2 files changed, 16 insertions(+), 2 deletions(-)
[PATCH for 7.4.0] conf: Avoid double indentation of <metadata/> element
Posted by Michal Privoznik 2 years, 11 months ago 
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@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);
         xmlIndentTreeOutput = oldIndentTreeOutput;
     }
 
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index a9eadff29c..20c6dc091a 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2488,6 +2488,7 @@ virNetworkDefFormatBuf(virBuffer *buf,
 
     if (def->metadata) {
         g_autoptr(xmlBuffer) xmlbuf = NULL;
+        const char *xmlbufContent = NULL;
         int oldIndentTreeOutput = xmlIndentTreeOutput;
 
         /* Indentation on output requires that we previously set
@@ -2504,7 +2505,13 @@ virNetworkDefFormatBuf(virBuffer *buf,
             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);
         xmlIndentTreeOutput = oldIndentTreeOutput;
     }
 
-- 
2.26.3
Re: [PATCH for 7.4.0] conf: Avoid double indentation of <metadata/> element
Posted by Pavel Hrdina 2 years, 11 months ago 
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@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);
>          xmlIndentTreeOutput = oldIndentTreeOutput;
>      }
>  
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index a9eadff29c..20c6dc091a 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -2488,6 +2488,7 @@ virNetworkDefFormatBuf(virBuffer *buf,
>  
>      if (def->metadata) {
>          g_autoptr(xmlBuffer) xmlbuf = NULL;
> +        const char *xmlbufContent = NULL;
>          int oldIndentTreeOutput = xmlIndentTreeOutput;
>  
>          /* Indentation on output requires that we previously set
> @@ -2504,7 +2505,13 @@ virNetworkDefFormatBuf(virBuffer *buf,
>              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);

Please create a helper function and put that workaround there instead of
having it open-coded at two different places.

Otherwise looks good.

Pavel

>          xmlIndentTreeOutput = oldIndentTreeOutput;
>      }
>  
> -- 
> 2.26.3
>
Re: [PATCH for 7.4.0] conf: Avoid double indentation of <metadata/> element
Posted by Bjoern Walk 2 years, 11 months ago 
Michal Privoznik <mprivozn@redhat.com> [2021-05-25, 09:32AM +0200]:
> 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@redhat.com>

Tested-by: Bjoern Walk <bwalk@linux.ibm.com>

But like Daniel said, this now breaks for older libxml2 versions.
Re: [PATCH for 7.4.0] conf: Avoid double indentation of <metadata/> element
Posted by Bjoern Walk 2 years, 11 months ago 
Bjoern Walk <bwalk@linux.ibm.com> [2021-05-25, 11:01AM +0200]:
> Michal Privoznik <mprivozn@redhat.com> [2021-05-25, 09:32AM +0200]:
> > 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@redhat.com>
> 
> Tested-by: Bjoern Walk <bwalk@linux.ibm.com>
> 
> But like Daniel said, this now breaks for older libxml2 versions.

Ah, I also misread this. All is fine with both libxml2 v2.9.12 and
earlier.

-- 
IBM Systems
Linux on Z & KVM Development
--------------------------------------------------
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
--------------------------------------------------
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH for 7.4.0] conf: Avoid double indentation of <metadata/> element
Posted by Daniel P. Berrangé 2 years, 11 months ago 
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@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 ?


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 :|
Re: [PATCH for 7.4.0] conf: Avoid double indentation of <metadata/> element
Posted by Michal Prívozník 2 years, 11 months ago 
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@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.

Michal

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.