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

[Michal Prívozník]

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.