[libvirt] [PATCH v4] virsh: Increase device-detach intelligence
From: Michal Prívozník <mprivozn@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(-) diff --git a/tools/virsh.c b/tools/virsh.c index 430168c..3e87dbb 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -59,6 +59,7 @@ #include "threads.h" #include "command.h" #include "virkeycode.h" +#include "util/bitmap.h" static char *progname; @@ -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. + * @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; + + /* 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)){ + 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; + } + + if (!(bitmap = virBitmapAlloc(n1_child_size))) { + virReportOOMError(); + return false; + } + + child2 = n2->children; + while (child2) { + if (child2->type != XML_ELEMENT_NODE) { + child2 = child2->next; + continue; + } + + child1 = n1->children; + n1_iter = 0; + found = false; + while (child1) { + if (child1->type != XML_ELEMENT_NODE) { + child1 = child1->next; + continue; + } + + if (virBitmapGetBit(bitmap, n1_iter, &visited) < 0) { + vshError(NULL, "%s", _("Bad child elements counting.")); + goto cleanup; + } + + if (visited) { + child1 = child1->next; + n1_iter++; + continue; + } + + if (xmlStrEqual(child1->name, child2->name)) { + found = true; + if (virBitmapSetBit(bitmap, n1_iter) < 0) { + vshError(NULL, "%s", _("Bad child elements counting.")); + goto cleanup; + } + + if (!vshNodeIsSuperset(child1, child2)) + goto cleanup; + + break; + } + + child1 = child1->next; + n1_iter++; + } + + if (!found) + goto cleanup; + + child2 = child2->next; + } + + ret = true; + +cleanup: + virBitmapFree(bitmap); + return ret; +} + +/** + * vshCompleteXMLFromDomain: + * @ctl vshControl for error messages printing + * @dom domain + * @oldXML device XML before + * @newXML and after completion + * + * To given domain and (probably incomplete) device XML specification try to + * find such device in domain and complete missing parts. This is however + * possible only when given device XML is sufficiently precise so it addresses + * only one device. + * + * 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; + } + + domDoc = virXMLParseStringCtxt(domXML, _("(domain definition)"), &domCtxt); + if (!domDoc) { + vshError(ctl, _("Failed to parse domain definition xml")); + goto cleanup; + } + + devDoc = virXMLParseStringCtxt(oldXML, _("(device definition)"), &devCtxt); + if (!devDoc) { + vshError(ctl, _("Failed to parse device definition xml")); + goto cleanup; + } + + node = xmlDocGetRootElement(devDoc); + + 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; + int indx = -1; + for (i = 0; i < devices_size; i++) { + if (vshNodeIsSuperset(devices[i], node)) { + if (indx >= 0) { + funcRet = -3; /* ambiguous */ + goto cleanup; + } + indx = i; + } + } + + if (indx < 0) { + funcRet = -2; /* no such device */ + goto cleanup; + } + + vshDebug(ctl, VSH_ERR_DEBUG, "Found device at pos %d\n", indx); + + 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); + buf->content = NULL; + } + + vshDebug(ctl, VSH_ERR_DEBUG, "Old xml:\n%s\nNew xml:\n%s\n", oldXML, *newXML); + + funcRet = 0; + +cleanup: + xmlBufferFree(buf); + VIR_FREE(devices); + xmlXPathFreeContext(devCtxt); + xmlXPathFreeContext(domCtxt); + xmlFreeDoc(devDoc); + xmlFreeDoc(domDoc); + VIR_FREE(domXML); + return funcRet; +} /* * "detach-device" command @@ -11006,10 +11238,11 @@ static const vshCmdOptDef opts_detach_device[] = { static bool cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) { - virDomainPtr dom; + virDomainPtr dom = NULL; const char *from = NULL; - char *buffer; + char *buffer = NULL, *new_buffer = NULL; int ret; + bool funcRet = false; unsigned int flags; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -11018,37 +11251,50 @@ 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 cleanup; if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { virshReportError(ctl); - virDomainFree(dom); - return false; + goto cleanup; + } + + 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")); + } else { + /* vshCompleteXMLFromDomain() already printed error message, + * so nothing to do here. */ + } + goto cleanup; } if (vshCommandOptBool(cmd, "persistent")) { flags = VIR_DOMAIN_AFFECT_CONFIG; if (virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_AFFECT_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")); + goto cleanup; } + vshPrint(ctl, "%s", _("Device detached successfully\n")); + funcRet = true; + +cleanup: + VIR_FREE(new_buffer); + VIR_FREE(buffer); virDomainFree(dom); - return true; + return funcRet; } -- 1.7.3.4
On 09/14/2011 08:46 AM, Michal Privoznik wrote:
From: Michal Prívozník<mprivozn@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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michal Privoznik