virsh: Strip XML declaration when extracting CPU XMLs

[libvirt] [PATCH] virsh: Strip XML declaration when extracting CPU XMLs

Posted by Jiri Denemark 5 years, 5 months ago

Since commit v4.3.0-336-gc84726fbdd all
{hypervisor-,}cpu-{baseline,compare} commands use a generic
vshExtractCPUDefXMLs helper for extracting individual CPU definitions
from the provided input file. The helper wraps the input file in a
<container> element so that several independent elements can be easily
parsed from the file. This works fine except when the file starts with
XML declaration (<?xml version="1.0" ... ?>) because the XML declaration
cannot be put inside any element. In fact it has to be at the very
beginning of the XML document without any preceding white space
characters. We can just simply skip the XML declaration.

https://bugzilla.redhat.com/show_bug.cgi?id=1595993

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 tools/virsh-host.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 16f504bafe..b7f86bdd91 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -1130,13 +1130,20 @@ vshExtractCPUDefXMLs(vshControl *ctl,
     xmlDocPtr xml = NULL;
     xmlXPathContextPtr ctxt = NULL;
     xmlNodePtr *nodes = NULL;
+    char *doc;
     size_t i;
     int n;
 
     if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &buffer) < 0)
         goto error;
 
-    if (virAsprintf(&xmlStr, "<container>%s</container>", buffer) < 0)
+    /* Strip possible XML declaration */
+    if (STRPREFIX(buffer, "<?xml") && (doc = strstr(buffer, "?>")))
+        doc += 2;
+    else
+        doc = buffer;
+
+    if (virAsprintf(&xmlStr, "<container>%s</container>", doc) < 0)
         goto error;
 
     if (!(xml = virXMLParseStringCtxt(xmlStr, xmlFile, &ctxt)))
-- 
2.19.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virsh: Strip XML declaration when extracting CPU XMLs

Posted by Ján Tomko 5 years, 5 months ago

On Thu, Nov 22, 2018 at 04:46:33PM +0100, Jiri Denemark wrote:
>Since commit v4.3.0-336-gc84726fbdd all
>{hypervisor-,}cpu-{baseline,compare} commands use a generic
>vshExtractCPUDefXMLs helper for extracting individual CPU definitions
>from the provided input file. The helper wraps the input file in a
><container> element so that several independent elements can be easily
>parsed from the file. This works fine except when the file starts with
>XML declaration (<?xml version="1.0" ... ?>) because the XML declaration
>cannot be put inside any element. In fact it has to be at the very
>beginning of the XML document without any preceding white space
>characters. We can just simply skip the XML declaration.

What if someone specifies a doctype? O:)
Also, does libvirt produce such files? I don't think we should bother
doing extra work to undo the extra work done by the user.

>
>https://bugzilla.redhat.com/show_bug.cgi?id=1595993
>

I only see a relation between the bug summary and this patch.
There's no mention of the XML declaration there and no mention of the
other issues mentioned there here in the patch.

Jano

>Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
>---
> tools/virsh-host.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virsh: Strip XML declaration when extracting CPU XMLs

Posted by Jiri Denemark 5 years, 5 months ago

On Thu, Nov 22, 2018 at 17:39:16 +0100, Ján Tomko wrote:
> On Thu, Nov 22, 2018 at 04:46:33PM +0100, Jiri Denemark wrote:
> >Since commit v4.3.0-336-gc84726fbdd all
> >{hypervisor-,}cpu-{baseline,compare} commands use a generic
> >vshExtractCPUDefXMLs helper for extracting individual CPU definitions
> >from the provided input file. The helper wraps the input file in a
> ><container> element so that several independent elements can be easily
> >parsed from the file. This works fine except when the file starts with
> >XML declaration (<?xml version="1.0" ... ?>) because the XML declaration
> >cannot be put inside any element. In fact it has to be at the very
> >beginning of the XML document without any preceding white space
> >characters. We can just simply skip the XML declaration.
> 
> What if someone specifies a doctype? O:)
> Also, does libvirt produce such files? I don't think we should bother
> doing extra work to undo the extra work done by the user.

Of course, we don't generate XML declarations, but I can imagine tools
formatting DOM into a file could just automatically output the XML
declaration unless you explicitly tell them not to do so. On the other
hand, nothing would output a doctype or <?xml-stylesheet ?> without an
explicit action.

Moreover, libvirt itself doesn't mind XML declarations and virsh before
4.4.0 didn't mind either.

I agree, it's probably a corner case and I was thinking about not fixing
it, but it turned out to be really easy thanks to the strict XML
specification.

> >https://bugzilla.redhat.com/show_bug.cgi?id=1595993
> 
> I only see a relation between the bug summary and this patch.
> There's no mention of the XML declaration there and no mention of the
> other issues mentioned there here in the patch.

Oops, the patch is supposed to fix another bug
https://bugzilla.redhat.com/show_bug.cgi?id=1592737

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virsh: Strip XML declaration when extracting CPU XMLs

Posted by Ján Tomko 5 years, 4 months ago

On Fri, Nov 23, 2018 at 12:48:44PM +0100, Jiri Denemark wrote:
>On Thu, Nov 22, 2018 at 17:39:16 +0100, Ján Tomko wrote:
>> On Thu, Nov 22, 2018 at 04:46:33PM +0100, Jiri Denemark wrote:
>> >Since commit v4.3.0-336-gc84726fbdd all
>> >{hypervisor-,}cpu-{baseline,compare} commands use a generic
>> >vshExtractCPUDefXMLs helper for extracting individual CPU definitions
>> >from the provided input file. The helper wraps the input file in a
>> ><container> element so that several independent elements can be easily
>> >parsed from the file. This works fine except when the file starts with
>> >XML declaration (<?xml version="1.0" ... ?>) because the XML declaration
>> >cannot be put inside any element. In fact it has to be at the very
>> >beginning of the XML document without any preceding white space
>> >characters. We can just simply skip the XML declaration.
>>
>> What if someone specifies a doctype? O:)
>> Also, does libvirt produce such files? I don't think we should bother
>> doing extra work to undo the extra work done by the user.
>
>Of course, we don't generate XML declarations, but I can imagine tools
>formatting DOM into a file could just automatically output the XML
>declaration unless you explicitly tell them not to do so. On the other
>hand, nothing would output a doctype or <?xml-stylesheet ?> without an
>explicit action.
>
>Moreover, libvirt itself doesn't mind XML declarations and virsh before
>4.4.0 didn't mind either.
>
>I agree, it's probably a corner case and I was thinking about not fixing
>it, but it turned out to be really easy thanks to the strict XML
>specification.
>
>> >https://bugzilla.redhat.com/show_bug.cgi?id=1595993
>>
>> I only see a relation between the bug summary and this patch.
>> There's no mention of the XML declaration there and no mention of the
>> other issues mentioned there here in the patch.
>
>Oops, the patch is supposed to fix another bug
>https://bugzilla.redhat.com/show_bug.cgi?id=1592737
>

With the bug link fixed:

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list