Re: [Libvir] [PATCH] check file format in virsh attach/detach-device

Masayuki Sunou wrote: 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