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

[Eric Blake]

On 09/14/2011 08:46 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:
> -change the superset search
> -tweak little snippets acording to Eric's review
>
>   tools/virsh.c |  278 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>   1 files changed, 262 insertions(+), 16 deletions(-)
>
> @@ -10986,6 +10987,237 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd)
>       return true;
>   }
>
> +/**
> + * Check if n1 is superset of n2, meaning n1 contains all elements and
> + * attributes as n2 at lest. Including children.

s/lest/least/

> + * @n1 first node
> + * @n2 second node
> + * return 1 in case n1 covers n2, 0 otherwise.
> + */
> +static bool
> +vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) {
> +    xmlNodePtr child1, child2;
> +    xmlAttrPtr attr;
> +    char *prop1, *prop2;
> +    bool found;
> +    bool visited;
> +    bool ret = false;
> +    unsigned long n1_child_size, n1_iter;
> +    virBitmapPtr bitmap;
> +
> +    if (!n1&&  !n2)
> +        return true;
> +
> +    if (!n1 || !n2)
> +        return false;
> +
> +    if (!xmlStrEqual(n1->name, n2->name))
> +        return true;

return false - if the names differ, there is no superset relation.

> +
> +    /* Iterate over n2 attributes and check if n1 contains them*/
> +    attr = n2->properties;
> +    while (attr) {
> +        if (attr->type == XML_ATTRIBUTE_NODE) {
> +            prop1 = virXMLPropString(n1, (const char *) attr->name);
> +            prop2 = virXMLPropString(n2, (const char *) attr->name);
> +            if (STRNEQ_NULLABLE(prop1, prop2)){

Space before { (at least, this one looks like a real spacing issue in 
your patch, and not yet another instance of thunderbird mangling things 
in my reply).

> +                xmlFree(prop1);
> +                xmlFree(prop2);
> +                return false;
> +            }
> +            xmlFree(prop1);
> +            xmlFree(prop2);
> +        }
> +        attr = attr->next;
> +    }
> +
> +    n1_child_size = xmlChildElementCount(n1);
> +    if (!n1_child_size) {
> +        return (n1->type == XML_ELEMENT_NODE) ? true : false;

I'm not a fan of 'return bool_cond ? true : false', when 'return 
bool_cond' gives the same results.

I'm not sure this line is right.  If n1 has no children, then it is a 
superset of n2 only if n2 has no children, not if n1 is a node.  Maybe 
you were trying to do this short circuit:  If n2 has no children, then 
it doesn't matter how many children n1 has, and you can return true at 
this point.

Also, why are you checking n1->type here?  Shouldn't we already know 
that n1->type and n2->type are both XML_ELEMENT_NODE before this 
function is ever called?

> + *
> + * To given domain and (probably incomplete) device XML specification try to

s/To given/For a given/

> +
> +    domDoc = virXMLParseStringCtxt(domXML, _("(domain definition)"),&domCtxt);

Peter's recent patch means that you need to:
s/domain definition/domain_definition/

> +    if (!domDoc) {
> +        vshError(ctl, _("Failed to parse domain definition xml"));
> +        goto cleanup;
> +    }
> +
> +    devDoc = virXMLParseStringCtxt(oldXML, _("(device definition)"),&devCtxt);

s/device definition/device_definition/

> +
> +    buf = xmlBufferCreate();

Why are we using xmlBuffer instead of virBuf APIs?  Not necessarily 
wrong, but also not typical in the rest of our code.

> +    if (!buf) {
> +        vshError(ctl, "%s", _("out of memory"));
> +        goto cleanup;
> +    }
> +
> +    xmlBufferCat(buf, BAD_CAST "/domain/devices/");
> +    xmlBufferCat(buf, node->name);

In fact, you could probably do the same in fewer lines of code, with:

virAsprintf(xpath, "/domain/devices/%s", node->name)

> +        goto cleanup;
> +    }
> +
> +    /* and refine */
> +    int i = 0;
> +    int indx = -1;

gcc didn't warn about declarations after goto?  I would float the 
declaration of i and indx earlier in the function, to match our style.

> +    if (newXML) {
> +        sctxt = xmlSaveToBuffer(buf, NULL, 0);
> +        if (!sctxt) {
> +            vshError(ctl, "%s", _("failed to create document saving context"));
> +            goto cleanup;
> +        }
> +
> +        xmlSaveTree(sctxt, devices[indx]);
> +        xmlSaveClose(sctxt);
> +        *newXML = (char *) xmlBufferContent(buf);

if (!*newXML) report OOM and goto cleanup, with funcRet of -1

> +        buf->content = NULL;
> +    }
> +
> +    vshDebug(ctl, VSH_ERR_DEBUG, "Old xml:\n%s\nNew xml:\n%s\n", oldXML, *newXML);

Null deref unless you use: newXML ? NULLSTR(*newXML) : "(null)"

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org