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

[Masayuki Sunou]

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