Ug... and trying to "locally" fix the RW/RO discovered two more issues...
On 06/11/2014 08:21 AM, John Ferlan wrote:
>
> 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
>
<...snip...>
>> +
>> +/*
>> + *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;
Needs to be initialized to NULL (char **ret = NULL;)
>> + 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;
Use:
+ } else {
+ *roDisksPath = ret;
}
>> + return nodeSize;
>> +}
>> +
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list