On 06/06/2011 04:20 AM, Michal Prívozník wrote: > 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 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 d98be1c..5c2634c 100644 > --- a/tools/virsh.c > +++ b/tools/virsh.c > @@ -9302,6 +9302,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. s/lest/least/ > + * @n1 first node > + * @n2 second node > + * return 1 in case n1 covers n2, 0 otherwise. You're using this as a bool, so make it bool. Return true for superset, false for mismatch. > + */ > +static int s/int/bool/ > +vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) { > + xmlNodePtr child1, child2; > + xmlAttrPtr attr1, attr2; > + int found; Looks like you are also using found as a bool. > + > + if (!n1 && !n2) > + return 1; s/1/true/ > + > + if (!n1 || !n2) > + return 0; s/0/false/, etc. > + > + if (!xmlStrEqual(n1->name, n2->name)) > + return 0; > + > + /* Iterate over n2 attributes and check if n1 contains them*/ style: space before comment end: "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))) Why xmlStrEqual and the nasty casting? Why not just STREQ(), since virXMLPropString is already char*? > + 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; > + } This probably won't work for XML that can be <oneOrMore> in the rng schema, especially if you also have <interleave> in the mix. For example, these two should be treated as equivalent (both serve as a superset of the other), but your algorithm will compare n2->b[0] with n1->b[0], followed by n2->b[1] with n1->b[0]: <a> <b>one</b> <b>two</b> </a> <a> <b>two</b> <b>one</b> </a> Thankfully, I don't think we are really using any <interleave> or <oneOrMore> subelements in any of the xml elements you plan on comparing via this function, but what happens when that changes? > + 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 s/To given/For a given/ > + * 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))) { Commit b9e51e56 recently changed a lot of xmlReadDoc to the simpler virXMLParseString; this should do likewise. > + /* 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)); Is this memmove necessary (it scales quadratically)? Or would it be better to leave the devices array untouched, and instead initialize 'int index = -1' before the loop, and on match if it is still -1 then set it to the match index otherwise error out with -3 (ambiguous), and after the loop error out with -2 (missing) if it is still -1 (this would scale linearly). > + 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. */ That's awkward. Generally, a helper function should either print no messages, or print all possible messages. But I guess it works here because of the distinct return values, even though it is a bit harder to audit the division of labor. -- Eric Blake eblake@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list