[libvirt] [PATCH] Revert "virsh: Increase device-detach intelligence"
This reverts commit ea7182c29f185e7c1527b10fbe16dc4ba1f45a88. Conflicts are resolved. This is a temporary reverting for it introduces regression of device detaching (any device XML doesn't uses the same order as libvirt generates, or has uses decimal for some attrs (e.g. "slot") will cause the failure, as virsh compares the XML simply earlier before internal parsing). We could take the patch back when the new API for normalizing XML is ready. --- tools/virsh.c | 292 +++----------------------------------------------------- 1 files changed, 16 insertions(+), 276 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 04c1f6e..fb940b8 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -60,7 +60,6 @@ #include "command.h" #include "virkeycode.h" #include "virnetdevbandwidth.h" -#include "util/bitmap.h" #include "conf/domain_conf.h" static char *progname; @@ -12571,251 +12570,6 @@ 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 least. Including children. - * @n1 first node - * @n2 second node - * returns true in case n1 covers n2, false otherwise. - */ -static bool -vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) -{ - xmlNodePtr child1, child2; - xmlAttrPtr attr; - char *prop1, *prop2; - bool found; - bool visited; - bool ret = false; - long n1_child_size, n2_child_size, n1_iter; - virBitmapPtr bitmap; - - if (!n1 && !n2) - return true; - - if (!n1 || !n2) - return false; - - if (!xmlStrEqual(n1->name, n2->name)) - return false; - - /* 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 = virXMLChildElementCount(n1); - n2_child_size = virXMLChildElementCount(n2); - if (n1_child_size < 0 || n2_child_size < 0 || - n1_child_size < n2_child_size) - return false; - - if (n1_child_size == 0 && n2_child_size == 0) - return true; - - 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 - * - * For 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 = NULL; - xmlBufferPtr buf = NULL; - int i = 0; - int indx = -1; - - 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; - } - - /* Get all possible devices */ - virAsprintf(&xpath, "/domain/devices/%s", node->name); - if (!xpath) { - virReportOOMError(); - goto cleanup; - } - devices_size = virXPathNodeSet(xpath, domCtxt, &devices); - - 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 */ - 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); - if (!*newXML) { - virReportOOMError(); - goto cleanup; - } - buf->content = NULL; - } - - vshDebug(ctl, VSH_ERR_DEBUG, "Old xml:\n%s\nNew xml:\n%s\n", oldXML, - newXML ? NULLSTR(*newXML) : "(null)"); - - funcRet = 0; - -cleanup: - xmlBufferFree(buf); - VIR_FREE(devices); - xmlXPathFreeContext(devCtxt); - xmlXPathFreeContext(domCtxt); - xmlFreeDoc(devDoc); - xmlFreeDoc(domDoc); - VIR_FREE(domXML); - VIR_FREE(xpath); - return funcRet; -} - /* * "detach-device" command */ @@ -12835,11 +12589,10 @@ static const vshCmdOptDef opts_detach_device[] = { static bool cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) { - virDomainPtr dom = NULL; + virDomainPtr dom; const char *from = NULL; - char *buffer = NULL, *new_buffer = NULL; + char *buffer; int ret; - bool funcRet = false; unsigned int flags; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -12848,50 +12601,37 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) - goto cleanup; + if (vshCommandOptString(cmd, "file", &from) <= 0) { + virDomainFree(dom); + return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { virshReportError(ctl); - 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; + virDomainFree(dom); + return false; } if (vshCommandOptBool(cmd, "persistent")) { flags = VIR_DOMAIN_AFFECT_CONFIG; if (virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_AFFECT_LIVE; - ret = virDomainDetachDeviceFlags(dom, new_buffer, flags); + ret = virDomainDetachDeviceFlags(dom, buffer, flags); } else { - ret = virDomainDetachDevice(dom, new_buffer); + ret = virDomainDetachDevice(dom, buffer); } + VIR_FREE(buffer); if (ret < 0) { vshError(ctl, _("Failed to detach device from %s"), from); - goto cleanup; + virDomainFree(dom); + return false; + } else { + vshPrint(ctl, "%s", _("Device detached successfully\n")); } - vshPrint(ctl, "%s", _("Device detached successfully\n")); - funcRet = true; - -cleanup: - VIR_FREE(new_buffer); - VIR_FREE(buffer); virDomainFree(dom); - return funcRet; + return true; } -- 1.7.7.3
On 01/11/2012 04:47 AM, Osier Yang wrote:
This reverts commit ea7182c29f185e7c1527b10fbe16dc4ba1f45a88.
Conflicts are resolved.
This is a temporary reverting for it introduces regression of device detaching (any device XML doesn't uses the same order as libvirt generates, or has uses decimal for some attrs (e.g. "slot") will cause the failure, as virsh compares the XML simply earlier before internal parsing). We could take the patch back when the new API for normalizing XML is ready. --- tools/virsh.c | 292 +++----------------------------------------------------- 1 files changed, 16 insertions(+), 276 deletions(-)
Do we really want to flat-out revert this entire patch?
-/** - * 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 - * returns true in case n1 covers n2, false otherwise. - */ -static bool -vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) -{
This function seems like we would still be using it if we add the new API for normalizing XML; so reverting it just to re-add it later doesn't make as much sense as just leaving it in place and not using it for now (but you may have to add a compiler attribute to mark it unused in the meantime).
-/** - * vshCompleteXMLFromDomain: - * @ctl vshControl for error messages printing - * @dom domain - * @oldXML device XML before - * @newXML and after completion - * - * For 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)
This was the function where Dan suggested the idea of taking (normalized) oldXML, and turning it into a couple of XPath queries of the domain XML. For example, if the user passes only a MAC address, that would translate into an XPath query of how many interface/mac elements exist, and if that is exactly one, then grab the interface node that contains the given interface/mac value. So we may be rewriting this anyways, at which point it is no different if we keep this function around unused or wait for the rewrite.
@@ -12835,11 +12589,10 @@ static const vshCmdOptDef opts_detach_device[] = { static bool cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) { - virDomainPtr dom = NULL; + virDomainPtr dom; const char *from = NULL; - char *buffer = NULL, *new_buffer = NULL; + char *buffer; int ret; - bool funcRet = false; unsigned int flags;
Some of these cleanups, such as setting up funcRet to a default value and adding a cleanup label, were independently useful. I'm not sure whether reverting the entire patch, or just the portion associated with the problems of subset matching, makes more sense. That is,
- - 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; + virDomainFree(dom); + return false; }
this would be the only hunk that really matters, rather than the entire patch, while still keeping all the other improvements of the original, and keeping the attempt around for comparing against a future patch that does a better comparison job. I'm inclined to say: Don't push this patch until we have the replacement ready to go. What we have now may not be 100% working, but it does work in some cases, while reverting now with no replacement takes us back to 0% working. In other words, I think that using this patch in isolation is wrong; and would feel more comfortable ack-ing it as part of a larger series that replaces things with a better version. There's still time to get such a series written before 0.9.10. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2012年01月12日 01:34, Eric Blake wrote:
On 01/11/2012 04:47 AM, Osier Yang wrote:
This reverts commit ea7182c29f185e7c1527b10fbe16dc4ba1f45a88.
Conflicts are resolved.
This is a temporary reverting for it introduces regression of device detaching (any device XML doesn't uses the same order as libvirt generates, or has uses decimal for some attrs (e.g. "slot") will cause the failure, as virsh compares the XML simply earlier before internal parsing). We could take the patch back when the new API for normalizing XML is ready. --- tools/virsh.c | 292 +++----------------------------------------------------- 1 files changed, 16 insertions(+), 276 deletions(-)
Do we really want to flat-out revert this entire patch?
-/** - * 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 - * returns true in case n1 covers n2, false otherwise. - */ -static bool -vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) -{
This function seems like we would still be using it if we add the new API for normalizing XML; so reverting it just to re-add it later doesn't make as much sense as just leaving it in place and not using it for now (but you may have to add a compiler attribute to mark it unused in the meantime).
Agreed
-/** - * vshCompleteXMLFromDomain: - * @ctl vshControl for error messages printing - * @dom domain - * @oldXML device XML before - * @newXML and after completion - * - * For 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)
This was the function where Dan suggested the idea of taking (normalized) oldXML, and turning it into a couple of XPath queries of the domain XML. For example, if the user passes only a MAC address, that would translate into an XPath query of how many interface/mac elements exist, and if that is exactly one, then grab the interface node that contains the given interface/mac value. So we may be rewriting this anyways, at which point it is no different if we keep this function around unused or wait for the rewrite.
@@ -12835,11 +12589,10 @@ static const vshCmdOptDef opts_detach_device[] = { static bool cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) { - virDomainPtr dom = NULL; + virDomainPtr dom; const char *from = NULL; - char *buffer = NULL, *new_buffer = NULL; + char *buffer; int ret; - bool funcRet = false; unsigned int flags;
Some of these cleanups, such as setting up funcRet to a default value and adding a cleanup label, were independently useful. I'm not sure whether reverting the entire patch, or just the portion associated with the problems of subset matching, makes more sense. That is,
- - 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; + virDomainFree(dom); + return false; }
this would be the only hunk that really matters, rather than the entire patch, while still keeping all the other improvements of the original, and keeping the attempt around for comparing against a future patch that does a better comparison job.
Agreed.
I'm inclined to say:
Don't push this patch until we have the replacement ready to go. What we have now may not be 100% working, but it does work in some cases, while reverting now with no replacement takes us back to 0% working. In other words, I think that using this patch in isolation is wrong; and would feel more comfortable ack-ing it as part of a larger series that replaces things with a better version. There's still time to get such a series written before 0.9.10.
Agreed.
participants (2)
-
Eric Blake -
Osier Yang