[libvirt] [PATCH v3] virsh: Increase device-detach intelligence
Eric Blake
eblake at redhat.com
Tue Sep 13 21:32:14 UTC 2011
On 08/24/2011 06:53 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.
Limited to just virsh, so if you can polish this up to v4, this could
still be useful to push pre-0.9.5.
>
> +/**
> + * Check if n1 is superset of n2, meaning n1 contains all elements and
> + * attributes as n2 at lest. Including children.
> + * @n1 first node
> + * @n2 second node
> + * return 1 in case n1 covers n2, 0 otherwise.
> + */
> +static int
s/int/bool/
> +vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) {
> + xmlNodePtr child1, child2;
> + xmlAttrPtr attr1, attr2;
> + int found;
> +
> + if (!n1&& !n2)
> + return 1;
s/1/true/, and so forth.
> +
> + if (!n1 || !n2)
> + return 0;
> +
> + if (!xmlStrEqual(n1->name, n2->name))
> + return 0;
> +
> + /* Iterate over n2 attributes and check if n1 contains them*/
> + attr2 = n2->properties;
> + while (attr2) {
> + if (attr2->type == XML_ATTRIBUTE_NODE) {
> + attr1 = n1->properties;
> + found = 0;
> + while (attr1) {
> + if (xmlStrEqual(attr1->name, attr2->name)) {
> + found = 1;
> + break;
> + }
> + attr1 = attr1->next;
> + }
> + if (!found)
> + return 0;
> + if (!xmlStrEqual(BAD_CAST virXMLPropString(n1, (const char *) attr1->name),
> + BAD_CAST virXMLPropString(n2, (const char *) attr2->name)))
Memory leak. virXMLPropString() allocates, so you must free the
results. Also, xmlStrEqual is painful to use; why not just use STREQ(),
to get rid of those BAD_CAST?
> + return 0;
> + }
> + attr2 = attr2->next;
> + }
> +
> + /* and now iterate over n2 children */
> + child2 = n2->children;
> + while (child2) {
> + if (child2->type == XML_ELEMENT_NODE) {
> + child1 = n1->children;
> + found = 0;
> + while (child1) {
> + if (child1->type == XML_ELEMENT_NODE&&
> + xmlStrEqual(child1->name, child2->name)) {
> + found = 1;
> + break;
> + }
Ouch. Whereas attributes can be reordered at will, children are not
necessarily as flexible. There are cases where a parent can have
multiple children by the same name, and where order is important (for
example, the order of multiple <disk> matters), but there are also cases
where we intentionally allow interleaving (within <disk>, you can
specify <source> or <target> first and still validate). Your code is
doing a quadratic search for the first matching child (for every child
of n2, search through every child of n1 for a match), which covers the
interleave case, but not the ordered repetition case. I think the
safest way to do things is to keep a bitmap of which children we have
seen, to ensure that each child of n1 is mapped to exactly one child of
n2. Something like:
count how many children in n1
create bitmap that large
for each child in n2
for each child in n1
if bitmap is marked
continue inner loop
if name matches
mark bitmap
recursively compare the two children
continue outer loop
abort outer loop since no match found
cleanup bitmap
> + child1 = child1->next;
> + }
> + if (!found)
> + return 0;
> + if (!vshNodeIsSuperset(child1, child2))
> + return 0;
> + }
> + child2 = child2->next;
> + }
> +
> + return 1;
> +}
> +
> +/**
> + * To given domain and (probably incomplete) device XML specification try to
Grammar. Also, if you're going to use doxygen markup, then you need to
start with the function name:
/**
* vshCompleteXMLFromDomain:
* For a given domain and (probably incomplete) device XML
* specification, try to
> + * find such device in domain and complete missing parts. This is however
> + * possible when given device XML is sufficiently precise so it addresses only
This is possible only when the given device XML is sufficiently precise
that it addresses
> + * one device.
> + * @ctl vshControl for error messages printing
> + * @dom domain
> + * @oldXML device XML before
> + * @newXML and after completion
> + * Returns -2 when no such device exists in domain, -3 when given XML selects many
> + * (is too ambiguous), 0 in case of success. Otherwise returns -1. @newXML
> + * is touched only in case of success.
> + */
> +static int
> +vshCompleteXMLFromDomain(vshControl *ctl, virDomainPtr dom, char *oldXML,
> + char **newXML) {
> + 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;
> + xmlBufferPtr buf = NULL;
> +
> + if (!(domXML = virDomainGetXMLDesc(dom, 0))) {
> + vshError(ctl, _("couldn't get XML description of domain %s"),
> + virDomainGetName(dom));
> + goto cleanup;
> + }
> +
> + if (!(domDoc = xmlReadDoc(BAD_CAST domXML, "domain.xml", NULL,
> + XML_PARSE_NOENT | XML_PARSE_NONET |
> + XML_PARSE_NOERROR | XML_PARSE_NOWARNING))) {
> + vshError(ctl, "%s", _("could not parse domain XML"));
> + goto cleanup;
> + }
> +
> + if (!(devDoc = xmlReadDoc(BAD_CAST oldXML, "device.xml", NULL,
Didn't we just fix things to pass a more sensible name for xml parsed
from in-memory strings rather than an actual file?
> + XML_PARSE_NOENT | XML_PARSE_NONET |
> + XML_PARSE_NOERROR | XML_PARSE_NOWARNING))) {
> + vshError(ctl, "%s", _("could not parse device XML"));
> + goto cleanup;
> + }
> +
> + node = xmlDocGetRootElement(domDoc);
> + if (!node) {
> + vshError(ctl, "%s", _("failed to get domain root element"));
> + goto cleanup;
> + }
> +
> + domCtxt = xmlXPathNewContext(domDoc);
> + if (!domCtxt) {
> + vshError(ctl, "%s", _("failed to create context on domain XML"));
> + goto cleanup;
> + }
> + domCtxt->node = node;
> +
> + node = xmlDocGetRootElement(devDoc);
> + if (!node) {
> + vshError(ctl, "%s", _("failed to get device root element"));
> + goto cleanup;
> + }
I'm wondering if this can be simplified by using the newer
virXMLParseStringCtxt() macro in util/xml.h.
> +
> + devCtxt = xmlXPathNewContext(devDoc);
> + if (!devCtxt) {
> + vshError(ctl, "%s", _("failed to create context on device XML"));
> + goto cleanup;
> + }
> + devCtxt->node = node;
> +
> + buf = xmlBufferCreate();
> + if (!buf) {
> + vshError(ctl, "%s", _("out of memory"));
> + goto cleanup;
> + }
> +
> + xmlBufferCat(buf, BAD_CAST "/domain/devices/");
> + xmlBufferCat(buf, node->name);
> + xpath = (char *) xmlBufferContent(buf);
> + /* Get all possible devices */
> + devices_size = virXPathNodeSet(xpath, domCtxt,&devices);
> + xmlBufferEmpty(buf);
> +
> + if (devices_size< 0) {
> + /* error */
> + vshError(ctl, "%s", _("error when selecting nodes"));
> + goto cleanup;
> + } else if (devices_size == 0) {
> + /* no such device */
> + funcRet = -2;
> + goto cleanup;
> + }
> +
> + /* and refine */
> + int i = 0;
> + while (i< devices_size) {
> + if (!vshNodeIsSuperset(devices[i], node)) {
> + if (devices_size == 1) {
> + VIR_FREE(devices);
> + devices_size = 0;
> + } else {
> + memmove(devices + i, devices + i + 1,
> + sizeof(*devices) * (devices_size-i-1));
Using memmove on the tail of devices seems a bit more awkward than
iterating a pointer over members of devices in the first place.
> + devices_size--;
> + if (VIR_REALLOC_N(devices, devices_size)< 0) {
> + /* ignore, harmless */
> + }
> + }
> + } else {
> + i++;
> + }
> + }
I think I would have done this logic as:
devices = virXPathNodeSet
index = -1
for (i = 0; i < devices; i++)
if vshNodeIsSuperset(devices[i], node)
if index >= 0
ret = -3; goto cleanup /* ambiguous */
index = i;
if index < 0
ret = -2; goto cleanup /* no match */
ret = 0 /* match */
> +
> + if (!devices_size) {
> + /* no such device */
> + funcRet = -2;
> + goto cleanup;
> + } else if (devices_size> 1) {
> + /* ambiguous */
> + funcRet = -3;
> + goto cleanup;
> + }
> +
> + if (newXML) {
> + sctxt = xmlSaveToBuffer(buf, NULL, 0);
> + if (!sctxt) {
> + vshError(ctl, "%s", _("failed to create document saving context"));
> + goto cleanup;
> + }
> +
> + xmlSaveTree(sctxt, devices[0]);
> + xmlSaveClose(sctxt);
> + *newXML = (char *) xmlBufferContent(buf);
> + buf->content = NULL;
> + }
> +
> + funcRet = 0;
> +
> +cleanup:
> + xmlBufferFree(buf);
> + VIR_FREE(devices);
> + xmlXPathFreeContext(devCtxt);
> + xmlXPathFreeContext(domCtxt);
> + xmlFreeDoc(devDoc);
> + xmlFreeDoc(domDoc);
> + VIR_FREE(domXML);
> + return funcRet;
> +}
>
> /*
> * "detach-device" command
> @@ -10371,10 +10591,11 @@ static const vshCmdOptDef opts_detach_device[] = {
> static bool
> cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
Everything below here looked fine; the problems remain in the earlier
code that picks out the proper xml snippet.
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list