Re: [libvirt] [PATCH] virsh: Make <mac> required when device-detaching interface

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.

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