Re: [libvirt] qemuDomainSnapshotLoad leak
Chris Lalancette wrote:
On 04/07/2010 11:47 AM, Jim Meyering wrote:
> clang reports that the assignment to "snap" below is a dead store.
> Actually it's also a leak, and it seems like it deserves a diagnostic
> if/when that function returns NULL.
It is a dead store, that is true, but it's not a leak. The pointer that
is returned there is a pointer to the internal snapshot object, which
we want to keep around. I guess we can just do:
virDomainSnapshotAssignDef(&vm->snapshots, def);
And don't do the assignment at all.
Hi Chris.
Thanks for the quick feedback.
Skipping the assignment altogether sounds good.
And yes, we should also check for
NULL and print an error if it fails.
After poking around a little, I would prefer not to give
a diagnostic when it returns NULL, since in 2 of 3 NULL-return
paths virDomainSnapshotAssignDef already issues a diagnostic.
Doing that is also consistent with the other use of that function
in the same file.
Here's the patch:
From ba6e3fe2c47c882a5b428b506b693c7d1ea4532a Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Wed, 7 Apr 2010 20:22:20 +0200
Subject: [PATCH] qemuDomainSnapshotLoad: avoid dead store
* src/qemu/qemu_driver.c (qemuDomainSnapshotLoad): Remove dead store
into "snap", as well as its declaration.
---
src/qemu/qemu_driver.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d1684b8..f5cf1f1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1354,7 +1354,6 @@ static void qemuDomainSnapshotLoad(void *payload,
char *xmlStr;
int ret;
char *fullpath;
- virDomainSnapshotObjPtr snap = NULL;
virDomainSnapshotDefPtr def = NULL;
char ebuf[1024];
@@ -1406,7 +1405,7 @@ static void qemuDomainSnapshotLoad(void *payload,
continue;
}
- snap = virDomainSnapshotAssignDef(&vm->snapshots, def);
+ virDomainSnapshotAssignDef(&vm->snapshots, def);
VIR_FREE(xmlStr);
}
--
1.7.1.rc0.212.gbd88f