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 looks to me like this function should return something other than "void", so that it can inform its caller of such a failure. No? This is from qemu_driver.c: static void qemuDomainSnapshotLoad(void *payload, const char *name ATTRIBUTE_UNUSED, void *data) { ... virDomainSnapshotObjPtr snap = NULL; ... while ((entry = readdir(dir))) { ... def = virDomainSnapshotDefParseString(xmlStr, 0); if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR("Failed to parse snapshot XML from file '%s'", fullpath); VIR_FREE(xmlStr); continue; } snap = virDomainSnapshotAssignDef(&vm->snapshots, def); VIR_FREE(xmlStr); } /* FIXME: qemu keeps internal track of snapshots. We can get access * to this info via the "info snapshots" monitor command for running * domains, or via "qemu-img snapshot -l" for shutoff domains. It would * be nice to update our internal state based on that, but there is a * a problem. qemu doesn't track all of the same metadata that we do. * In particular we wouldn't be able to fill in the <parent>, which is * pretty important in our metadata. */ virResetLastError(); cleanup: if (dir) closedir(dir); VIR_FREE(snapDir); virDomainObjUnlock(vm); }
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. And yes, we should also check for NULL and print an error if it fails.
It looks to me like this function should return something other than "void", so that it can inform its caller of such a failure.
No?
...but no, we should leave this function returning void. If something (anything) fails in here, there is nothing a higher level can do. We don't want to fail to start libvirtd because of one corrupt/bogus snapshot metadata file, so the most we can do is print an error message and go on. -- Chris Lalancette
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@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
On 04/07/2010 02:26 PM, Jim Meyering wrote:
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); }
Yeah, this looks perfectly good. ACK -- Chris Lalancette
participants (2)
-
Chris Lalancette -
Jim Meyering