On 22.11.2011 21:29, Eric Blake wrote: > On 11/22/2011 03:26 AM, Michal Privoznik wrote: >> From: Michal Prívozník <mpriv...@redhat.com> >> >> 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. >
Fixed and pushed. Thanks. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
