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

From: Michal Prívozník <mprivozn(a)redhat.com&gt; 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 v2: -rebase to current HEAD diff to v1: -rebase to current HEAD -add a little bit comments tools/virsh.c | 266 +++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 250 insertions(+), 16 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 1ad84a2..aae8e4e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10351,6 +10351,226 @@ 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; + + /* 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))) + 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; + } + 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 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, + 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; + } + + 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)); + devices_size--; + if (VIR_REALLOC_N(devices, devices_size) < 0) { + /* ignore, harmless */ + } + } + } else { + i++; + } + } + + 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) { - 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)) @@ -10383,37 +10604,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; }