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

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.