Re: [libvirt] [PATCH v8 2/4] Add vbox_snapshot_conf struct
One more here too - missed it in my last pass...
On 05/19/2014 08:47 AM, Yohan BELLEGUIC wrote:
...snip...
+
+/*
+ *isCurrentSnapshot: Return 1 if 'snapshotName' corresponds to the
+ *vboxSnapshotXmlMachinePtr's current snapshot, return 0 otherwise.
+ */
+int virVBoxSnapshotConfIsCurrentSnapshot(virVBoxSnapshotConfMachinePtr
machine, char *snapshotName)
+{
+virVBoxSnapshotConfSnapshotPtr snapshot = NULL;
+if (machine == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+ _(Machine is null));
+goto cleanup;
+}
+if (snapshotName == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+ _(snapshotName is null));
+goto cleanup;
+}
+snapshot = virVBoxSnapshotConfSnapshotByName(machine-snapshot,
snapshotName);
Coverity complains:
(5) Event returned_null:virVBoxSnapshotConfSnapshotByName returns
null (checked 4 out of 5 times). [details]
(14) Event var_assigned:Assigning: snapshot = null return value from
virVBoxSnapshotConfSnapshotByName.
You will need a:
if (snapshot == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_(Unable to find the snapshot %s), snapshotName);
goto cleanup;
}
+return STREQ(snapshot-uuid, machine-currentSnapshot);
(15) Event dereference: Dereferencing a null pointer snapshot.
+
+ cleanup:
+return 0;
+}
+
+/*
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v8 2/4] Add vbox_snapshot_conf struct
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v8 2/4] Add vbox_snapshot_conf struct
On Mon, May 19, 2014 at 02:47:31PM +0200, Yohan BELLEGUIC wrote:
This structure contains the data to be saved in the VirtualBox XML file
and can be manipulated with severals exposed functions.
The structure is created by vboxSnapshotLoadVboxFile taking the
machine XML file.
It also can rewrite the XML by using vboxSnapshotSaveVboxFile.
---
po/POTFILES.in |1 +
src/Makefile.am|1 +
src/vbox/vbox_snapshot_conf.c | 1490
src/vbox/vbox_snapshot_conf.h | 105 ++
tests/Makefile.am | 15 +
tests/vboxsnapshotxmldata/2disks-1snap.vbox| 322 +
tests/vboxsnapshotxmldata/2disks-2snap.vbox| 478 +++
.../vboxsnapshotxmldata/2disks-3snap-brother.vbox | 786 +++
tests/vboxsnapshotxmldata/2disks-3snap.vbox| 636 +
tests/vboxsnapshotxmldata/2disks-nosnap.vbox | 168 +++
tests/vboxsnapshotxmltest.c| 161 +++
11 files changed, 4163 insertions(+)
create mode 100644 src/vbox/vbox_snapshot_conf.c
create mode 100644 src/vbox/vbox_snapshot_conf.h
create mode 100644 tests/vboxsnapshotxmldata/2disks-1snap.vbox
create mode 100644 tests/vboxsnapshotxmldata/2disks-2snap.vbox
create mode 100644 tests/vboxsnapshotxmldata/2disks-3snap-brother.vbox
create mode 100644 tests/vboxsnapshotxmldata/2disks-3snap.vbox
create mode 100644 tests/vboxsnapshotxmldata/2disks-nosnap.vbox
create mode 100644 tests/vboxsnapshotxmltest.c
ACK
I made a few changes to this - primarily to add 'const' to all
the 'char *' strings, tweak line breaks, and remove some bogus
use of virStringFreeList on non-NULL terminated arrays
So the change I'm pushing includes this:
diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c
index 9fee760..4909665 100644
--- a/src/vbox/vbox_snapshot_conf.c
+++ b/src/vbox/vbox_snapshot_conf.c
@@ -30,9 +30,10 @@
#define VIR_FROM_THIS VIR_FROM_VBOX
VIR_LOG_INIT(vbox.vbox_snapshot_conf);
-static virVBoxSnapshotConfHardDiskPtr
virVBoxSnapshotConfCreateVBoxSnapshotConfHardDiskPtr(xmlNodePtr diskNode,
-
xmlXPathContextPtr xPathContext,
- char
*machineLocation)
+static virVBoxSnapshotConfHardDiskPtr
+virVBoxSnapshotConfCreateVBoxSnapshotConfHardDiskPtr(xmlNodePtr diskNode,
+ xmlXPathContextPtr
xPathContext,
+ const char
*machineLocation)
{
virVBoxSnapshotConfHardDiskPtr hardDisk = NULL;
xmlNodePtr *nodes = NULL;
@@ -114,9 +115,10 @@ static virVBoxSnapshotConfHardDiskPtr
virVBoxSnapshotConfCreateVBoxSnapshotConfH
return hardDisk;
}
-static virVBoxSnapshotConfMediaRegistryPtr
virVBoxSnapshotConfRetrieveMediaRegistry(xmlNodePtr mediaRegistryNode,
-
xmlXPathContextPtr xPathContext,
- char
*machineLocation)
+static virVBoxSnapshotConfMediaRegistryPtr
+virVBoxSnapshotConfRetrieveMediaRegistry(xmlNodePtr mediaRegistryNode,
+ xmlXPathContextPtr xPathContext,
+ const char *machineLocation)
{
virVBoxSnapshotConfMediaRegistryPtr mediaRegistry = NULL;
xmlNodePtr hardDisksNode = NULL;
@@ -175,8 +177,9 @@ static virVBoxSnapshotConfMediaRegistryPtr
virVBoxSnapshotConfRetrieveMediaRegis
return mediaRegistry;
}
-static virVBoxSnapshotConfSnapshotPtr
virVBoxSnapshotConfRetrieveSnapshot(xmlNodePtr snapshotNode,
- xmlXPathContextPtr
xPathContext)
+static virVBoxSnapshotConfSnapshotPtr
+virVBoxSnapshotConfRetrieveSnapshot(xmlNodePtr snapshotNode,
+xmlXPathContextPtr xPathContext)
{
virVBoxSnapshotConfSnapshotPtr snapshot = NULL;
xmlNodePtr hardwareNode = NULL;
@@ -278,8 +281,9 @@ static virVBoxSnapshotConfSnapshotPtr
virVBoxSnapshotConfRetrieveSnapshot(xmlNod
return snapshot;
}
-virVBoxSnapshotConfSnapshotPtr
virVBoxSnapshotConfSnapshotByName(virVBoxSnapshotConfSnapshotPtr snapshot,
- char *snapshotName)
+virVBoxSnapshotConfSnapshotPtr
+virVBoxSnapshotConfSnapshotByName(virVBoxSnapshotConfSnapshotPtr snapshot,
+ const char *snapshotName)
{
size_t i = 0;
virVBoxSnapshotConfSnapshotPtr ret = NULL;
@@ -293,8 +297,9 @@ virVBoxSnapshotConfSnapshotPtr
virVBoxSnapshotConfSnapshotByName(virVBoxSnapshot
return ret;
}
-static virVBoxSnapshotConfHardDiskPtr
virVBoxSnapshotConfHardDiskById(virVBoxSnapshotConfHardDiskPtr disk,
-
