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
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list