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
On 02/23/2011 05:30 AM, Daniel Veillard wrote: 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.