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