[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]