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

[Eric Blake]

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.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111122/b82a28fc/attachment-0001.sig>