[libvirt] [PATCH v4] virsh: Increase device-detach intelligence

[Michal Privoznik]

On 22.11.2011 21:29, Eric Blake wrote:
> On 11/22/2011 03:26 AM, Michal Privoznik wrote:
>> From: Michal Prívozník <mprivozn at redhat.com>
>>
>> Up to now users have to give a full XML description on input when
>> device-detaching. If they omitted something it lead to unclear
>> error messages (like generated MAC wasn't found, etc.).
>> With this patch users can specify only those information which
>> specify one device sufficiently precise. Remaining information is
>> completed from domain.
>> ---
>>
>> diff to v3:
>> - Eric's review: https://www.redhat.com/archives/libvir-list/2011-September/msg00472.html
> 
> Been a while since we last looked at this :)  Looks like you correctly
> implemented my algorithm suggestions from the v3 comments.
> 
>>  
>> +/**
>> + * Check if n1 is superset of n2, meaning n1 contains all elements and
>> + * attributes as n2 at least. Including children.
>> + * @n1 first node
>> + * @n2 second node
>> + * return 1 in case n1 covers n2, 0 otherwise.
>> + */
>> +static bool
> 
> Comment needs a tweak:
> 
> returns true in case n1 covers n2, false otherwise
> 
>> +
>> +    n1_child_size = xmlChildElementCount(n1);
>> +    if (!n1_child_size) {
>> +        return !xmlChildElementCount(n2);
>> +    }
> 
> Rewriting this chunk will give you a minor optimization in more
> situations.  For example, if n1 has two children but n2 has three
> children (to be a subset, n2 cannot have any more children than n1):
> 
> n1_child_size = xmlChildElementCount(n1);
> n2_child_size = xmlChildElementCount(n2);
> if (n1_child_size < n2_child_size)
>     return false;
> 
>> +static int
>> +vshCompleteXMLFromDomain(vshControl *ctl, virDomainPtr dom, char *oldXML,
>> +                         char **newXML) {
> 
> We haven't always been consistent, but generally the open { of a
> function appears on column 1 of a new line (same comment for
> vshNodeIsSuperset).
> 
>> +    int funcRet = -1;
>> +    char *domXML = NULL;
>> +    xmlDocPtr domDoc = NULL, devDoc = NULL;
>> +    xmlNodePtr node = NULL;
>> +    xmlXPathContextPtr domCtxt = NULL, devCtxt = NULL;
>> +    xmlNodePtr *devices = NULL;
>> +    xmlSaveCtxtPtr sctxt = NULL;
>> +    int devices_size;
>> +    char *xpath;
> 
> To avoid crash on early error, this must be:
> 
> char *xpath = NULL;
> 
>> +    buf = xmlBufferCreate();
>> +    if (!buf) {
>> +        vshError(ctl, "%s", _("out of memory"));
>> +        goto cleanup;
>> +    }
>> +
>> +    /* Get all possible devices */
>> +    virAsprintf(&xpath, "/domain/devices/%s", node->name);
>> +    if (!xpath) {
>> +        virReportOOMError();
>> +        goto cleanup;
> 
> since xpath is allocated on some, but not all paths, into cleanup.
> 
>> +
>> +cleanup:
>> +    xmlBufferFree(buf);
>> +    VIR_FREE(devices);
>> +    xmlXPathFreeContext(devCtxt);
>> +    xmlXPathFreeContext(domCtxt);
>> +    xmlFreeDoc(devDoc);
>> +    xmlFreeDoc(domDoc);
>> +    VIR_FREE(domXML);
>> +    return funcRet;
> 
> Mem leak if you don't have:
> 
> VIR_FREE(xpath);
> 
> ACK with those problems fixed.
> 

Fixed and pushed. Thanks.