Re: [libvirt] [PATCH v8 2/4] Add vbox_snapshot_conf struct

This patch has resulted in many new Coverity errors - mostly resource leaks as a result of the virVBoxSnapshotConfAllChildren() recursive function. I would clean them up, but I'm a bit leary of missing some nuance in the original design. There were also a couple of issues in virVBoxSnapshotConfGetRWDisksPathsFromLibvirtXML and virVBoxSnapshotConfGetRODisksPathsFromLibvirtXML Details of the issues and some thoughts are inline below in the functions These should be cleaned up before the next release... John

> + > +/* > + *getRWDisksPathsFromLibvirtXML: Parse a libvirt XML snapshot file, allocates and > + *fills a list of read-write disk paths. > + *return array length on success, -1 on failure. > + */ > +int virVBoxSnapshotConfGetRWDisksPathsFromLibvirtXML(char *filePath, char ***rwDisksPath) > +{ > + int result = -1; > + size_t i = 0; > + char **ret;

> + xmlDocPtr xml = NULL; > + xmlXPathContextPtr xPathContext = NULL; > + xmlNodePtr *nodes = NULL; > + int nodeSize = 0; > + if (filePath == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("filePath is null")); > + goto cleanup; > + } > + xml = virXMLParse(filePath, NULL, NULL); > + if (xml == NULL) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Unable to parse the xml")); > + goto cleanup; > + } > + if (!(xPathContext = xmlXPathNewContext(xml))) { > + virReportOOMError(); > + goto cleanup; > + } > + xPathContext->node = xmlDocGetRootElement(xml); > + nodeSize = virXPathNodeSet("/domainsnapshot/disks/disk", xPathContext, &nodes); Coverity comlains that nodeSize can be a negative number. Other code in libvirt will do something like: if ((nodeSize = virXPathNodeSet("/domainsnapshot/disks/disk", xPathContext, &nodes)) < 0) goto cleanup; > + > + if (VIR_ALLOC_N(ret, nodeSize) < 0) > + goto cleanup; > + > + for (i = 0; i < nodeSize; i++) { > + xmlNodePtr node = nodes[i]; > + xPathContext->node = node; > + xmlNodePtr sourceNode = virXPathNode("./source", xPathContext); > + if (sourceNode) { > + ret[i] = virXMLPropString(sourceNode, "file"); > + } > + } > + result = 0; > + > + cleanup: > + xmlFreeDoc(xml); > + xmlXPathFreeContext(xPathContext); > + if (result < 0) { > + virStringFreeList(ret); > + nodeSize = -1; > + } > + *rwDisksPath = ret; > + return nodeSize; > +} > + > +/* > + *getRODisksPathsFromLibvirtXML: *Parse a libvirt XML snapshot file, allocates and fills > + *a list of read-only disk paths (the parents of the read-write disks). > + *return array length on success, -1 on failure. > + */ > +int virVBoxSnapshotConfGetRODisksPathsFromLibvirtXML(char *filePath, char ***roDisksPath) > +{ > + int result = -1; > + size_t i = 0; > + char **ret; > + xmlDocPtr xml = NULL; > + xmlXPathContextPtr xPathContext = NULL; > + xmlNodePtr *nodes = NULL; > + int nodeSize = 0; > + if (filePath == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("filePath is null")); > + goto cleanup; > + } > + xml = virXMLParse(filePath, NULL, NULL); > + if (xml == NULL) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Unable to parse the xml")); > + goto cleanup; > + } > + if (!(xPathContext = xmlXPathNewContext(xml))) { > + virReportOOMError(); > + goto cleanup; > + } > + xPathContext->node = xmlDocGetRootElement(xml); > + nodeSize = virXPathNodeSet("/domainsnapshot/domain/devices/disk", > + xPathContext, > + &nodes); Same issue here as the RW code - you need to handle nodeSize return: if ((nodeSize = virXPathNodeSet("/domainsnapshot/domain/devices/disk", xPathContext, &nodes)) < 0) goto cleanup; > + if (VIR_ALLOC_N(ret, nodeSize) < 0) > + goto cleanup; > + > + for (i = 0; i < nodeSize; i++) { > + xmlNodePtr node = nodes[i]; > + xPathContext->node = node; > + xmlNodePtr sourceNode = virXPathNode("./source", xPathContext); > + if (sourceNode) { > + ret[i] = virXMLPropString(sourceNode, "file"); > + } > + } > + result = 0; > + > + cleanup: > + xmlFreeDoc(xml); > + xmlXPathFreeContext(xPathContext); > + if (result < 0) { > + virStringFreeList(ret); > + nodeSize = -1; > + } > + *roDisksPath = ret;

> + return nodeSize; > +} > +