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

Up to now users have to give a full XML description on input when device-detaching. If they omited 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. --- tools/virsh.c | 260 +++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 245 insertions(+), 15 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 50ca50f..95d27f7 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -8702,6 +8702,224 @@ 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. + * @n1 first node + * @n2 second node + * return 1 in case n1 covers n2, 0 otherwise. + */ +static int +vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) { + xmlNodePtr child1, child2; + xmlAttrPtr attr1, attr2; + int found; + + if (!n1 && !n2) + return 1; + + if (!n1 || !n2) + return 0; + + if (!xmlStrEqual(n1->name, n2->name)) + return 0; + + 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))) + return 0; + } + attr2 = attr2->next; + } + + 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; + } + 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 + * find such device in domain and complete missing parts. This is however + * possible when given device XML is sufficiently precise so it addresses only + * 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 error; + } + + 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 error; + } + + if (!(devDoc = xmlReadDoc(BAD_CAST oldXML, "device.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOERROR | XML_PARSE_NOWARNING))) { + vshError(ctl, "%s", _("could not parse device XML")); + goto error; + } + + node = xmlDocGetRootElement(domDoc); + if (!node) { + vshError(ctl, "%s", _("failed to get domain root element")); + goto error; + } + + domCtxt = xmlXPathNewContext(domDoc); + if (!domCtxt) { + vshError(ctl, "%s", _("failed to create context")); + goto error; + } + domCtxt->node = node; + + node = xmlDocGetRootElement(devDoc); + if (!node) { + vshError(ctl, "%s", _("failed to get device root element")); + goto error; + } + + devCtxt = xmlXPathNewContext(devDoc); + if (!devCtxt) { + vshError(ctl, "%s", _("failed to create context")); + goto error; + } + devCtxt->node = node; + + buf = xmlBufferCreate(); + if (!buf) { + vshError(ctl, "%s", _("out of memory")); + goto error; + } + + xmlBufferCat(buf, BAD_CAST "/domain/devices/"); + xmlBufferCat(buf, node->name); + xpath = (char *) xmlBufferContent(buf); + /* we 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 error; + } else if (devices_size < 1) { + /* no such device */ + funcRet = -2; + goto error; + } + + /* 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)); + devices_size--; + if (VIR_REALLOC_N(devices, devices_size) < 0) { + /* ignore, harmless */ + } + } + } else { + i++; + } + } + + if (!devices_size) { + /* no such device */ + funcRet = -2; + goto error; + } else if (devices_size > 1) { + /* ambiguous */ + funcRet = -3; + goto error; + } + + if (newXML) { + sctxt = xmlSaveToBuffer(buf, NULL, 0); + if (!sctxt) { + vshError(ctl, "%s", _("failed to create document saving context")); + goto error; + } + + xmlSaveTree(sctxt, devices[0]); + xmlSaveClose(sctxt); + *newXML = (char *) xmlBufferContent(buf); + buf->content = NULL; + } + + funcRet = 0; + +error: + xmlBufferFree(buf); + VIR_FREE(devices); + xmlXPathFreeContext(devCtxt); + xmlXPathFreeContext(domCtxt); + xmlFreeDoc(devDoc); + xmlFreeDoc(domDoc); + VIR_FREE(domXML); + return funcRet; +} /* * "detach-device" command @@ -8724,7 +8942,7 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; const char *from = NULL; - char *buffer; + char *buffer = NULL, *new_buffer = NULL; int ret = TRUE; unsigned int flags; @@ -8734,37 +8952,49 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; - if (vshCommandOptString(cmd, "file", &from) <= 0) { - virDomainFree(dom); - return FALSE; - } + if (vshCommandOptString(cmd, "file", &from) <= 0) + goto error; if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { virshReportError(ctl); - virDomainFree(dom); - return FALSE; + goto error; + } + + ret = vshCompleteXMLFromDomain(ctl, dom, buffer, &new_buffer); + if (ret < 0) { + if (ret == -2) { + vshError(ctl, _("no such device in %s"), virDomainGetName(dom)); + } else if (ret == -3) { + vshError(ctl, "%s", _("given XML selects too many devices. " + "Please, be more specific")); + } + ret = FALSE; + goto error; } if (vshCommandOptBool(cmd, "persistent")) { flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG; if (virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; - ret = virDomainDetachDeviceFlags(dom, buffer, flags); + ret = virDomainDetachDeviceFlags(dom, new_buffer, flags); } else { - ret = virDomainDetachDevice(dom, buffer); + ret = virDomainDetachDevice(dom, new_buffer); } - VIR_FREE(buffer); if (ret < 0) { vshError(ctl, _("Failed to detach device from %s"), from); - virDomainFree(dom); - return FALSE; - } else { - vshPrint(ctl, "%s", _("Device detached successfully\n")); + ret = FALSE; + goto error; } + vshPrint(ctl, "%s", _("Device detached successfully\n")); + ret = TRUE; + +error: + VIR_FREE(new_buffer); + VIR_FREE(buffer); virDomainFree(dom); - return TRUE; + return ret; } -- 1.7.4

On 03/25/2011 09:09 AM, Michal Privoznik wrote:
Up to now users have to give a full XML description on input when device-detaching. If they omited 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. --- tools/virsh.c | 260 +++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 245 insertions(+), 15 deletions(-)
Not forgotten, but not quite yet to the top of my queue, either :( I guess having too many patches to review is a good sign that there is a lot of contribution going on.
diff --git a/tools/virsh.c b/tools/virsh.c index 50ca50f..95d27f7 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -8702,6 +8702,224 @@ 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. + * @n1 first node + * @n2 second node + * return 1 in case n1 covers n2, 0 otherwise. + */ +static int +vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) {
Are there any xml library functions that can already do this without you rewriting it from scratch?
+/** + * To 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 + * 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) {
My quick glance of just the documentation says that this looks like a nice helper, but I haven't reviewed it in depth. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 04/05/2011 12:14 AM, Eric Blake wrote:
On 03/25/2011 09:09 AM, Michal Privoznik wrote:
Up to now users have to give a full XML description on input when device-detaching. If they omited 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. --- tools/virsh.c | 260 +++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 245 insertions(+), 15 deletions(-)
Not forgotten, but not quite yet to the top of my queue, either :( I guess having too many patches to review is a good sign that there is a lot of contribution going on.
diff --git a/tools/virsh.c b/tools/virsh.c index 50ca50f..95d27f7 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -8702,6 +8702,224 @@ 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. + * @n1 first node + * @n2 second node + * return 1 in case n1 covers n2, 0 otherwise. + */ +static int +vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) {
Are there any xml library functions that can already do this without you rewriting it from scratch? No, there are not. I consulted this issue with Daniel Veillard.
+/** + * To 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 + * 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) {
My quick glance of just the documentation says that this looks like a nice helper, but I haven't reviewed it in depth.
participants (3)
-
Eric Blake
-
Michal Privoznik
-
Michal Prívozník