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 <[EMAIL PROTECTED]> "Re: [Libvir] [PATCH] check file format in virsh attach/detach-device" ""Richard W.M. Jones" <[EMAIL PROTECTED]>" 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list