On 22.11.2011 21:29, Eric Blake wrote:
On 11/22/2011 03:26 AM, Michal Privoznik wrote:
From: Michal Prívozník <mprivozn@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.