Re: [Libvir] [PATCH] check file format in virsh attach/detach-device
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(a)redhat.com>
"Re: [Libvir] [PATCH] check file format in virsh attach/detach-device"
""Richard W.M. Jones" <rjones(a)redhat.com>" 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