On 02/23/2011 05:30 AM, Daniel Veillard wrote:
On Tue, Feb 22, 2011 at 11:16:29AM +0100, Michal Privoznik wrote:
Problem is, if user does not specify mac address in input XML,
we generate a random one, which is why device-detach fails giving
a confusing error message. Therefore<mac> needs to be required.
Well if the domain only has one interface then if there is no<mac>
specified the semantic of the operation is still clear. Since it a very
frequent user case, I would rather not break something which was working
and has a clear semantic.
I couldn't agree more, but this is not how it works now anyway. Actually
this should be the patch for
https://bugzilla.redhat.com/show_bug.cgi?id=616721
But yeah, it would be better, if it works that way you've pointed out.
IMHO we should check if the domain has multiple interface first and
only raise a problem then. I think I saw such a patch a few weeks ago
possibly in a different context though.
---
tools/virsh.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 46 insertions(+), 11 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 2837e0f..dfb48d2 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -8580,9 +8580,12 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
virDomainPtr dom;
char *from;
char *buffer;
- int ret = TRUE;
+ int ret = FALSE;
int found;
unsigned int flags;
+ xmlDocPtr xml = NULL;
+ xmlXPathContextPtr ctxt = NULL;
+ int mac_cnt;
if (!vshConnectionUsability(ctl, ctl->conn))
return FALSE;
@@ -8592,14 +8595,41 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
from = vshCommandOptString(cmd, "file",&found);
if (!found) {
- virDomainFree(dom);
- return FALSE;
+ goto cleanup;
}
if (virFileReadAll(from, VIRSH_MAX_XML_FILE,&buffer)< 0) {
virshReportError(ctl);
- virDomainFree(dom);
- return FALSE;
+ goto cleanup;
+ }
+
+ xml = xmlReadDoc((const xmlChar *) buffer, "interface.xml", NULL,
+ XML_PARSE_NOENT | XML_PARSE_NONET |
+ XML_PARSE_NOWARNING);
+
+ if (!xml) {
+ vshError(ctl, "%s", _("input XML is not valid"));
+ goto cleanup;
+ }
Hum, "valid"in XML means conformant to the DTD ot to a schemas, and
that's not something xmlReadDoc checks. The best is to say something like
"input XML failed to parse"
i.e. it's likely to be a syntactic error, which should be reported more
precisely by libxml2
+ ctxt = xmlXPathNewContext(xml);
+ mac_cnt = virXPathNodeSet("/interface/mac", ctxt, NULL);
+
+ switch(mac_cnt) {
+ case 1:
+ break;
+
+ case 0:
+ case -1:
+ vshError(ctl, "%s", _("You must specify mac address in xml file"));
+ goto cleanup;
+ break;
+
+ default:
+ vshError(ctl, "%s", _("You must specify exactly one mac address in"
+ " xml file"));
+ goto cleanup;
+ break;
}
if (vshCommandOptBool(cmd, "persistent")) {
@@ -8610,18 +8640,23 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
} else {
ret = virDomainDetachDevice(dom, buffer);
}
- VIR_FREE(buffer);
if (ret< 0) {
+ ret = FALSE;
vshError(ctl, _("Failed to detach device from %s"), from);
- virDomainFree(dom);
- return FALSE;
- } else {
- vshPrint(ctl, "%s", _("Device detached successfully\n"));
+ goto cleanup;
}
+ vshPrint(ctl, "%s", _("Device detached successfully\n"));
+ ret = TRUE;
centralizing error handling is a good idea
+cleanup:
+ xmlXPathFreeContext(ctxt);
+ if (xml)
+ xmlFreeDoc(xml);
+ VIR_FREE(buffer);
virDomainFree(dom);
- return TRUE;
+ return ret;
}
Daniel
I'll rewrite and sent v2.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list