Hi Daniel, Rich
Thanks for reviewing and suggestion.
I adopt (b) in consideration Daniel's suggestion.
--> Error message exists already.
This patch fixes virParseXMLDevice() for attach-device
and virDomainXMLDevID() for detach-device.
Thanks,
Masayuki Sunou.
----------------------------------------------------------------------
Index: src/xml.c
===================================================================
RCS file: /data/cvs/libvirt/src/xml.c,v
retrieving revision 1.86
diff -u -p -r1.86 xml.c
--- src/xml.c 31 Jul 2007 14:27:12 -0000 1.86
+++ src/xml.c 2 Aug 2007 05:41:21 -0000
@@ -1400,8 +1400,10 @@ virParseXMLDevice(virConnectPtr conn, ch
xml = xmlReadDoc((const xmlChar *) xmldesc, "domain.xml", NULL,
XML_PARSE_NOENT | XML_PARSE_NONET |
XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
- if (xml == NULL)
+ if (xml == NULL) {
+ virXMLError(conn, VIR_ERR_XML_ERROR, NULL, 0);
goto error;
+ }
node = xmlDocGetRootElement(xml);
if (node == NULL)
goto error;
@@ -1457,8 +1459,10 @@ virDomainXMLDevID(virDomainPtr domain, c
xml = xmlReadDoc((const xmlChar *) xmldesc, "domain.xml", NULL,
XML_PARSE_NOENT | XML_PARSE_NONET |
XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
- if (xml == NULL)
+ if (xml == NULL) {
+ virXMLError(NULL, VIR_ERR_XML_ERROR, NULL, 0);
goto error;
+ }
node = xmlDocGetRootElement(xml);
if (node == NULL)
goto error;
@@ -1499,6 +1503,9 @@ virDomainXMLDevID(virDomainPtr domain, c
goto error;
}
+ } else {
+ virXMLError(NULL, VIR_ERR_XML_ERROR, (const char *) node->name, 0);
+ goto error;
}
error:
ret = -1;
----------------------------------------------------------------------
In message <46AF1E04.2030703@xxxxxxxxxx>
"Re: [PATCH] check file format in virsh attach/detach-device"
""Richard W.M. Jones" <rjones@xxxxxxxxxx>" wrote:
> Masayuki Sunou wrote:
> > Hi
> >
> > virsh attach/detach-devich does not check file format now.
> >
> > This patch checks config file format is XML or not.
> > (in virsh attach/detach-device)
> > If file format is not XML, just return the error with message.
>
> Hello Masayuki,
>
> To summarise what your patch does:
>
> (1) It changes the semantics of virDomainAttachDevice and
> virDomainDetachDevice so that returning -2 as an error (instead of -1)
> means "the XML is invalid".
>
> (2) It changes the implementation of the xenDaemon{Attach,Detach}Device
> functions in xend_internal.c so that it returns -2 instead of -1 if
> virParseXMLDevice fails.
>
> My problems are:
>
> (1) is an ABI change (and undocumented!). In particular some of my own
> code depends on -1 meaning error (not just < 0). Really we can't accept
> patches which make ABI changes like this. You'll have seen a patch
> which I submitted last week get rejected because it made an egregious
> ABI change.
>
> (2) isn't quite correct. The function virParseXMLDevice can fail for
> many reasons (eg. out of memory).
>
> So I think a better solution would look like either:
>
> (a) Make virsh parse the XML file and do some simple sanity checks on
> it. I realise that virsh cannot do a full check on the XML, but it can
> at least check things like: Is it an XML file? Is the root node <disk>
> or <interface>? That should be enough to catch simple command-line errors.
>
> or:
>
> (b) Fix the error handling in virParseXMLDevice so that it generates
> reasonable errors. At the moment the error handling is mostly missing,
> and so error don't get propagated back to the user.
>
> or:
>
> (c) Modify libvirt to add a virDomainVerifyDeviceXML call which returns
> 0 (correct) or -1 (error) depending on whether the Device XML passed is
> reasonable. Then virsh can do this call before it does the
> attach/detach call.
>
> I hope that is helpful.
>
> Rich.
>
> --
> Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
> Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
> Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
> England and Wales under Company Registration No. 03798903
>
>
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list