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

On 11/22/2011 03:26 AM, Michal Privoznik wrote: > 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 v3: > - Eric's review: https://www.redhat.com/archives/libvir-list/2011-September/msg00472.html Been a while since we last looked at this :) Looks like you correctly implemented my algorithm suggestions from the v3 comments. > > +/** > + * 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 > + * return 1 in case n1 covers n2, 0 otherwise. > + */ > +static bool Comment needs a tweak: returns true in case n1 covers n2, false otherwise > + > + n1_child_size = xmlChildElementCount(n1); > + if (!n1_child_size) { > + return !xmlChildElementCount(n2); > + } Rewriting this chunk will give you a minor optimization in more situations. For example, if n1 has two children but n2 has three children (to be a subset, n2 cannot have any more children than n1): n1_child_size = xmlChildElementCount(n1); n2_child_size = xmlChildElementCount(n2); if (n1_child_size < n2_child_size) return false; > +static int > +vshCompleteXMLFromDomain(vshControl *ctl, virDomainPtr dom, char *oldXML, > + char **newXML) { We haven't always been consistent, but generally the open { of a function appears on column 1 of a new line (same comment for vshNodeIsSuperset). > + 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; To avoid crash on early error, this must be: char *xpath = NULL; > + 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; since xpath is allocated on some, but not all paths, into cleanup. > + > +cleanup: > + xmlBufferFree(buf); > + VIR_FREE(devices); > + xmlXPathFreeContext(devCtxt); > + xmlXPathFreeContext(domCtxt); > + xmlFreeDoc(devDoc); > + xmlFreeDoc(domDoc); > + VIR_FREE(domXML); > + return funcRet; Mem leak if you don't have: VIR_FREE(xpath); ACK with those problems fixed.