[libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
This addresses another coverity-spotted "flaw". However, since "cgroup" is never NULL after that initial "if" stmt, the only penalty is that the useless cleanup test would make a reviewer try to figure out how cgroup could be NULL there.
From d89098801d4e5011e07994cf0391ace2363d8971 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 17 May 2010 19:18:12 +0200 Subject: [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
* src/lxc/lxc_driver.c (lxcFreezeContainer): Remove test-after-deref. Correct indentation in expression. --- src/lxc/lxc_driver.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index fc0df37..8c3bbd3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2276,69 +2276,71 @@ static int lxcDomainSetAutostart(virDomainPtr dom, } } else { if (unlink(autostartLink) < 0 && errno != ENOENT && errno != ENOTDIR) { virReportSystemError(errno, _("Failed to delete symlink '%s'"), autostartLink); goto cleanup; } } vm->autostart = autostart; ret = 0; cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); if (vm) virDomainObjUnlock(vm); lxcDriverUnlock(driver); return ret; } static int lxcFreezeContainer(lxc_driver_t *driver, virDomainObjPtr vm) { int timeout = 1000; /* In milliseconds */ int check_interval = 1; /* In milliseconds */ int exp = 10; int waited_time = 0; int ret = -1; char *state = NULL; virCgroupPtr cgroup = NULL; if (!(driver->cgroup && - virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) == 0)) + virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) == 0)) return -1; + /* From here on, we know that cgroup != NULL. */ + while (waited_time < timeout) { int r; /* * Writing "FROZEN" to the "freezer.state" freezes the group, * i.e., the container, temporarily transiting "FREEZING" state. * Once the freezing is completed, the state of the group transits * to "FROZEN". * (see linux-2.6/Documentation/cgroups/freezer-subsystem.txt) */ r = virCgroupSetFreezerState(cgroup, "FROZEN"); /* * Returning EBUSY explicitly indicates that the group is * being freezed but incomplete and other errors are true * errors. */ if (r < 0 && r != -EBUSY) { VIR_DEBUG("Writing freezer.state failed with errno: %d", r); goto error; } if (r == -EBUSY) VIR_DEBUG0("Writing freezer.state gets EBUSY"); /* * Unfortunately, returning 0 (success) is likely to happen * even when the freezing has not been completed. Sometimes * the state of the group remains "FREEZING" like when * returning -EBUSY and even worse may never transit to * "FROZEN" even if writing "FROZEN" again. * * So we don't trust the return value anyway and always * decide that the freezing has been complete only with * the state actually transit to "FROZEN". @@ -2351,68 +2353,67 @@ static int lxcFreezeContainer(lxc_driver_t *driver, virDomainObjPtr vm) VIR_DEBUG("Reading freezer.state failed with errno: %d", r); goto error; } VIR_DEBUG("Read freezer.state: %s", state); if (STREQ(state, "FROZEN")) { ret = 0; goto cleanup; } waited_time += check_interval; /* * Increasing check_interval exponentially starting with * small initial value treats nicely two cases; One is * a container is under no load and waiting for long period * makes no sense. The other is under heavy load. The container * may stay longer time in FREEZING or never transit to FROZEN. * In that case, eager polling will just waste CPU time. */ check_interval *= exp; VIR_FREE(state); } VIR_DEBUG0("lxcFreezeContainer timeout"); error: /* * If timeout or an error on reading the state occurs, * activate the group again and return an error. * This is likely to fall the group back again gracefully. */ virCgroupSetFreezerState(cgroup, "THAWED"); ret = -1; cleanup: - if (cgroup) - virCgroupFree(&cgroup); + virCgroupFree(&cgroup); VIR_FREE(state); return ret; } static int lxcDomainSuspend(virDomainPtr dom) { lxc_driver_t *driver = dom->conn->privateData; virDomainObjPtr vm; virDomainEventPtr event = NULL; int ret = -1; lxcDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); lxcError(VIR_ERR_NO_DOMAIN, _("No domain with matching uuid '%s'"), uuidstr); goto cleanup; } if (!virDomainObjIsActive(vm)) { lxcError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running")); goto cleanup; } if (vm->state != VIR_DOMAIN_PAUSED) { if (lxcFreezeContainer(driver, vm) < 0) { lxcError(VIR_ERR_OPERATION_FAILED, "%s", _("Suspend operation failed")); goto cleanup; -- 1.7.1.250.g7d1e8
On 05/17/2010 11:22 AM, Jim Meyering wrote:
This addresses another coverity-spotted "flaw". However, since "cgroup" is never NULL after that initial "if" stmt, the only penalty is that the useless cleanup test would make a reviewer try to figure out how cgroup could be NULL there.
ACK.
cleanup: - if (cgroup) - virCgroupFree(&cgroup); + virCgroupFree(&cgroup);
Is this something that the useless-if-before-free test could have caught, rather than waiting for coverity? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/17/2010 11:22 AM, Jim Meyering wrote:
This addresses another coverity-spotted "flaw". However, since "cgroup" is never NULL after that initial "if" stmt, the only penalty is that the useless cleanup test would make a reviewer try to figure out how cgroup could be NULL there.
ACK.
Thanks.
cleanup: - if (cgroup) - virCgroupFree(&cgroup); + virCgroupFree(&cgroup);
Is this something that the useless-if-before-free test could have caught, rather than waiting for coverity?
Very good idea. Here's a patch to do that. With this, "make sc_avoid_if_before_free" (part of make syntax-check) will now prevent any new useless checks. The above was the sole offender.
From 387d3649eed2644576733febc53641228bb93257 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 17 May 2010 19:38:35 +0200 Subject: [PATCH] maint: add virCgroupFree to the list of free-like functions
This makes the useless-if-before-free test in maint.mk spot uses of virCgroupFree just like it does for free and the other listed functions. * cfg.mk (useless_free_options): Add virCgroupFree. Prompted by suggestion from Eric Blake. --- cfg.mk | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/cfg.mk b/cfg.mk index 7773d06..0b281a5 100644 --- a/cfg.mk +++ b/cfg.mk @@ -64,10 +64,11 @@ local-checks-to-skip = \ sc_makefile_check \ sc_useless_cpp_parens useless_free_options = \ --name=sexpr_free \ + --name=virCgroupFree \ --name=VIR_FREE \ --name=xmlFree \ --name=xmlXPathFreeContext \ --name=virDomainDefFree \ --name=xmlXPathFreeObject -- 1.7.1.250.g7d1e8
On 05/17/2010 11:40 AM, Jim Meyering wrote:
Eric Blake wrote:
On 05/17/2010 11:22 AM, Jim Meyering wrote:
This addresses another coverity-spotted "flaw". However, since "cgroup" is never NULL after that initial "if" stmt, the only penalty is that the useless cleanup test would make a reviewer try to figure out how cgroup could be NULL there.
ACK.
Thanks.
cleanup: - if (cgroup) - virCgroupFree(&cgroup); + virCgroupFree(&cgroup);
Hmm, on re-reading this, why is virCgroupFree taking a pointer to a virCgroupPtr, then blindly dereferencing it throughout the cleanup? It almost seems like we have the wrong signature, and should be using: virCgroupFree(virCgroupPtr group) instead of virCgroupFree(virCgroupPtr *group) and adjust all callers. But that's a separate cleanup.
This makes the useless-if-before-free test in maint.mk spot uses of virCgroupFree just like it does for free and the other listed functions. * cfg.mk (useless_free_options): Add virCgroupFree. Prompted by suggestion from Eric Blake.
ACK, given the current semantics of virCgroupFree. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/17/2010 11:40 AM, Jim Meyering wrote:
Eric Blake wrote:
On 05/17/2010 11:22 AM, Jim Meyering wrote:
This addresses another coverity-spotted "flaw". However, since "cgroup" is never NULL after that initial "if" stmt, the only penalty is that the useless cleanup test would make a reviewer try to figure out how cgroup could be NULL there.
ACK.
Thanks.
cleanup: - if (cgroup) - virCgroupFree(&cgroup); + virCgroupFree(&cgroup);
Hmm, on re-reading this, why is virCgroupFree taking a pointer to a virCgroupPtr, then blindly dereferencing it throughout the cleanup? It almost seems like we have the wrong signature, and should be using:
virCgroupFree(virCgroupPtr group)
instead of
virCgroupFree(virCgroupPtr *group)
and adjust all callers.
Almost. virCgroupPtr takes an address of a pointer so it can set the caller's pointer to NULL (via its VIR_FREE use). Otherwise, in order to retain existing semantics, some callers would have to change from this: virCgroupFree(&p); to this: virCgroupFree(p); p = NULL;
This makes the useless-if-before-free test in maint.mk spot uses of virCgroupFree just like it does for free and the other listed functions. * cfg.mk (useless_free_options): Add virCgroupFree. Prompted by suggestion from Eric Blake.
ACK, given the current semantics of virCgroupFree.
Thanks.
Jim Meyering wrote:
Eric Blake wrote:
On 05/17/2010 11:22 AM, Jim Meyering wrote:
This addresses another coverity-spotted "flaw". However, since "cgroup" is never NULL after that initial "if" stmt, the only penalty is that the useless cleanup test would make a reviewer try to figure out how cgroup could be NULL there.
ACK.
Thanks.
cleanup: - if (cgroup) - virCgroupFree(&cgroup); + virCgroupFree(&cgroup);
Is this something that the useless-if-before-free test could have caught, rather than waiting for coverity?
Very good idea. Here's a patch to do that. With this, "make sc_avoid_if_before_free" (part of make syntax-check) will now prevent any new useless checks. The above was the sole offender.
Actually, this is just the tip of the iceberg. A couple minutes work have shown that there are many more: Running this shows there are potentially at least 100 candidate functions: aid free|grep '^vi' Looking at the first few on the list, I've found that most are indeed free-like: N virBufferFreeAndReset y virCPUDefFree Y virCapabilitiesFree y virCapabilitiesFreeGuest y virCapabilitiesFreeGuestDomain y virCapabilitiesFreeGuestFeature y virCapabilitiesFreeGuestMachine y virCapabilitiesFreeHostNUMACell y virCapabilitiesFreeMachines N virCapabilitiesFreeNUMAInfo (should probably fix) y virCgroupFree N virConfFree (diagnoses the "error") y virConfFreeList y virConfFreeValue So far, most of the exceptions can be "fixed" to also be free-like. Using those few additions uncovered these: src/conf/storage_conf.c: if (pool->newDef) virStoragePoolDefFree(pool->newDef) src/security/virt-aa-helper.c: if (ctl->caps) virCapabilitiesFree(ctl->caps) src/util/conf.c: if (cur->value) { virConfFreeValue(cur->value); }
On 05/17/2010 11:53 AM, Jim Meyering wrote:
Very good idea. Here's a patch to do that. With this, "make sc_avoid_if_before_free" (part of make syntax-check) will now prevent any new useless checks. The above was the sole offender.
Actually, this is just the tip of the iceberg. A couple minutes work have shown that there are many more:
Running this shows there are potentially at least 100 candidate functions:
aid free|grep '^vi'
aid? Cool alias/shell script on just your machine? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/17/2010 11:53 AM, Jim Meyering wrote:
Very good idea. Here's a patch to do that. With this, "make sc_avoid_if_before_free" (part of make syntax-check) will now prevent any new useless checks. The above was the sole offender.
Actually, this is just the tip of the iceberg. A couple minutes work have shown that there are many more:
Running this shows there are potentially at least 100 candidate functions:
aid free|grep '^vi'
aid? Cool alias/shell script on just your machine?
It's a program from the id-utils package: http://www.gnu.org/software/id-utils/ You run "mkid" to create an index (or "make ID" in an automake-enabled project), then commands like aid, lid, gid, aid, eid, etc. do useful things. These tools used to be essential, but "git grep" and fast disks have come a long ways to rendering "gid" unnecessary. $ aid --help Usage: aid [OPTION]... PATTERN... Query ID database and report results. By default, output consists of multiple lines, each line containing the matched identifier followed by the list of file names in which it occurs. -f, --file=FILE file name of ID database -i, --ignore-case match PATTERN case insensitively -l, --literal match PATTERN as a literal string -r, --regexp match PATTERN as a regular expression -w, --word match PATTERN as a delimited word -s, --substring match PATTERN as a substring Note: If PATTERN contains extended regular expression meta- characters, it is interpreted as a regular expression substring. Otherwise, PATTERN is interpreted as a literal word. -k, --key=STYLE STYLE is one of `token', `pattern' or `none' -R, --result=STYLE STYLE is one of `filenames', `grep', `edit' or `none' -S, --separator=STYLE STYLE is one of `braces', `space' or `newline' and only applies to file names when `--result=filenames' The above STYLE options control how query results are presented. Defaults are --key=token --result=filenames --separator=space -F, --frequency=FREQ find tokens that occur FREQ times, where FREQ is a range expressed as `N..M'. If N is omitted, it defaults to 1, if M is omitted it defaults to MAX_USHRT -a, --ambiguous=LEN find tokens whose names are ambiguous for LEN chars -x, --hex only find numbers expressed as hexadecimal -d, --decimal only find numbers expressed as decimal -o, --octal only find numbers expressed as octal By default, searches match numbers of any radix. --help display this help and exit --version output version information and exit Report bugs to bug-idutils@gnu.org
On 05/17/2010 12:04 PM, Jim Meyering wrote:
aid free|grep '^vi'
aid? Cool alias/shell script on just your machine?
It's a program from the id-utils package:
http://www.gnu.org/software/id-utils/
You run "mkid" to create an index (or "make ID" in an automake-enabled project), then commands like aid, lid, gid, aid, eid, etc. do useful things.
Hmm, no id-utils in Fedora yet. I guess if I want to try it out, it's time for a self-compilation :) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Jim Meyering wrote:
Jim Meyering wrote:
Eric Blake wrote:
On 05/17/2010 11:22 AM, Jim Meyering wrote:
This addresses another coverity-spotted "flaw". However, since "cgroup" is never NULL after that initial "if" stmt, the only penalty is that the useless cleanup test would make a reviewer try to figure out how cgroup could be NULL there.
ACK.
Thanks.
cleanup: - if (cgroup) - virCgroupFree(&cgroup); + virCgroupFree(&cgroup);
Is this something that the useless-if-before-free test could have caught, rather than waiting for coverity?
Very good idea. Here's a patch to do that. With this, "make sc_avoid_if_before_free" (part of make syntax-check) will now prevent any new useless checks. The above was the sole offender.
Actually, this is just the tip of the iceberg. A couple minutes work have shown that there are many more:
Running this shows there are potentially at least 100 candidate functions:
aid free|grep '^vi'
Looking at the first few on the list, I've found that most are indeed free-like:
N virBufferFreeAndReset y virCPUDefFree Y virCapabilitiesFree y virCapabilitiesFreeGuest y virCapabilitiesFreeGuestDomain y virCapabilitiesFreeGuestFeature y virCapabilitiesFreeGuestMachine y virCapabilitiesFreeHostNUMACell y virCapabilitiesFreeMachines N virCapabilitiesFreeNUMAInfo (should probably fix) y virCgroupFree N virConfFree (diagnoses the "error") y virConfFreeList y virConfFreeValue
So far, most of the exceptions can be "fixed" to also be free-like.
Using those few additions uncovered these:
src/conf/storage_conf.c: if (pool->newDef) virStoragePoolDefFree(pool->newDef) src/security/virt-aa-helper.c: if (ctl->caps) virCapabilitiesFree(ctl->caps) src/util/conf.c: if (cur->value) { virConfFreeValue(cur->value); }
Here's a complete patch:
From 59d7134bc925de86b807262896a76256b889c96a Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 17 May 2010 22:38:59 +0200 Subject: [PATCH] maint: add more free-like functions to the list and deal with fallout
* cfg.mk (useless_free_options): Add many vir*Free* function names, and then remove the useless if-before-free tests exposed by running make syntax-check. * src/conf/interface_conf.c (virInterfaceDefFree): Remove useless "if". (virInterfaceAssignDef): Likewise. * src/conf/network_conf.c (virNetworkAssignDef): Likewise. * src/conf/storage_conf.c (virStoragePoolObjAssignDef): Likewise. * src/node_device/node_device_hal.c (dev_create): Likewise. * src/security/virt-aa-helper.c (vahDeinit): Likewise. * src/test/test_driver.c (testNodeDeviceCreateXML): Likewise. * src/util/conf.c (virConfSetValue): Likewise. --- cfg.mk | 163 +++++++++++++++++++++++++++++++++++-- src/conf/interface_conf.c | 13 +-- src/conf/network_conf.c | 3 +- src/conf/storage_conf.c | 3 +- src/node_device/node_device_hal.c | 3 +- src/security/virt-aa-helper.c | 3 +- src/test/test_driver.c | 3 +- src/util/conf.c | 4 +- 8 files changed, 167 insertions(+), 28 deletions(-) diff --git a/cfg.mk b/cfg.mk index 0b281a5..96d6953 100644 --- a/cfg.mk +++ b/cfg.mk @@ -64,15 +64,164 @@ local-checks-to-skip = \ sc_makefile_check \ sc_useless_cpp_parens -useless_free_options = \ - --name=sexpr_free \ - --name=virCgroupFree \ - --name=VIR_FREE \ - --name=xmlFree \ - --name=xmlXPathFreeContext \ - --name=virDomainDefFree \ +useless_free_options = \ + --name=VIR_FREE \ + --name=sexpr_free \ + --name=virCPUDefFree \ + --name=virCapabilitiesFree \ + --name=virCapabilitiesFreeGuest \ + --name=virCapabilitiesFreeGuestDomain \ + --name=virCapabilitiesFreeGuestFeature \ + --name=virCapabilitiesFreeGuestMachine \ + --name=virCapabilitiesFreeHostNUMACell \ + --name=virCapabilitiesFreeMachines \ + --name=virCgroupFree \ + --name=virConfFreeList \ + --name=virConfFreeValue \ + --name=virDomainChrDefFree \ + --name=virDomainControllerDefFree \ + --name=virDomainDefFree \ + --name=virDomainDeviceDefFree \ + --name=virDomainDiskDefFree \ + --name=virDomainEventCallbackListFree \ + --name=virDomainEventFree \ + --name=virDomainEventQueueFree \ + --name=virDomainFSDefFree \ + --name=virDomainGraphicsDefFree \ + --name=virDomainHostdevDefFree \ + --name=virDomainInputDefFree \ + --name=virDomainNetDefFree \ + --name=virDomainObjFree \ + --name=virDomainSnapshotDefFree \ + --name=virDomainSnapshotObjFree \ + --name=virDomainSoundDefFree \ + --name=virDomainVideoDefFree \ + --name=virDomainWatchdogDefFree \ + --name=virInterfaceDefFree \ + --name=virInterfaceIpDefFree \ + --name=virInterfaceObjFree \ + --name=virInterfaceProtocolDefFree \ + --name=virJSONValueFree \ + --name=virLastErrFreeData \ + --name=virNWFilterDefFree \ + --name=virNWFilterEntryFree \ + --name=virNWFilterHashTableFree \ + --name=virNWFilterIPAddrLearnReqFree \ + --name=virNWFilterIncludeDefFree \ + --name=virNWFilterPoolObjFree \ + --name=virNWFilterRuleDefFree \ + --name=virNWFilterRuleInstFree \ + --name=virNetworkDefFree \ + --name=virNetworkObjFree \ + --name=virNodeDeviceDefFree \ + --name=virNodeDeviceObjFree \ + --name=virSecretDefFree \ + --name=virStorageEncryptionFree \ + --name=virStorageEncryptionSecretFree \ + --name=virStoragePoolDefFree \ + --name=virStoragePoolObjFree \ + --name=virStoragePoolSourceFree \ + --name=virStorageVolDefFree \ + --name=xmlFree \ + --name=xmlXPathFreeContext \ --name=xmlXPathFreeObject +# The following template was generated by this command: +# make ID && aid free|grep '^vi'|sed 's/ .*//;s/^/# /' +# N virBufferFreeAndReset +# y virCPUDefFree +# y virCapabilitiesFree +# y virCapabilitiesFreeGuest +# y virCapabilitiesFreeGuestDomain +# y virCapabilitiesFreeGuestFeature +# y virCapabilitiesFreeGuestMachine +# y virCapabilitiesFreeHostNUMACell +# y virCapabilitiesFreeMachines +# N virCapabilitiesFreeNUMAInfo FIXME +# y virCgroupFree +# N virConfFree (diagnoses the "error") +# y virConfFreeList +# y virConfFreeValue +# y virDomainChrDefFree +# y virDomainControllerDefFree +# y virDomainDefFree +# y virDomainDeviceDefFree +# y virDomainDiskDefFree +# y virDomainEventCallbackListFree +# y virDomainEventFree +# y virDomainEventQueueFree +# y virDomainFSDefFree +# n virDomainFree +# n virDomainFreeName (can't fix -- returns int) +# y virDomainGraphicsDefFree +# y virDomainHostdevDefFree +# y virDomainInputDefFree +# y virDomainNetDefFree +# y virDomainObjFree +# y virDomainSnapshotDefFree +# n virDomainSnapshotFree (returns int) +# n virDomainSnapshotFreeName (returns int) +# y virDomainSnapshotObjFree +# y virDomainSoundDefFree +# y virDomainVideoDefFree +# y virDomainWatchdogDefFree +# n virDrvNodeGetCellsFreeMemory (returns int) +# n virDrvNodeGetFreeMemory (returns long long) +# n virFree (dereferences param) +# n virFreeError +# n virHashFree (takes 2 args) +# y virInterfaceDefFree +# n virInterfaceFree (returns int) +# n virInterfaceFreeName +# y virInterfaceIpDefFree +# y virInterfaceObjFree +# n virInterfaceObjListFree +# y virInterfaceProtocolDefFree +# y virJSONValueFree +# y virLastErrFreeData +# y virNWFilterDefFree +# y virNWFilterEntryFree +# n virNWFilterFree (returns int) +# y virNWFilterHashTableFree +# y virNWFilterIPAddrLearnReqFree +# y virNWFilterIncludeDefFree +# n virNWFilterPoolFreeName (returns int) +# y virNWFilterPoolObjFree +# n virNWFilterPoolObjListFree FIXME +# y virNWFilterRuleDefFree +# n virNWFilterRuleFreeInstanceData (typedef) +# y virNWFilterRuleInstFree +# y virNetworkDefFree +# n virNetworkFree (returns int) +# n virNetworkFreeName (returns int) +# y virNetworkObjFree +# n virNetworkObjListFree FIXME +# n virNodeDevCapsDefFree FIXME +# y virNodeDeviceDefFree +# n virNodeDeviceFree (returns int) +# y virNodeDeviceObjFree +# n virNodeDeviceObjListFree FIXME +# n virNodeGetCellsFreeMemory (returns int) +# n virNodeGetFreeMemory (returns non-void) +# y virSecretDefFree +# n virSecretFree (returns non-void) +# n virSecretFreeName (2 args) +# n virSecurityLabelDefFree FIXME +# n virStorageBackendDiskMakeFreeExtent (returns non-void) +# y virStorageEncryptionFree +# y virStorageEncryptionSecretFree +# n virStorageFreeType (enum) +# y virStoragePoolDefFree +# n virStoragePoolFree (returns non-void) +# n virStoragePoolFreeName (returns non-void) +# y virStoragePoolObjFree +# n virStoragePoolObjListFree FIXME +# y virStoragePoolSourceFree +# y virStorageVolDefFree +# n virStorageVolFree (returns non-void) +# n virStorageVolFreeName (returns non-void) +# n virStreamFree + # Avoid uses of write(2). Either switch to streams (fwrite), or use # the safewrite wrapper. sc_avoid_write: diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 920b090..6430f7a 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -85,20 +85,18 @@ void virInterfaceDefFree(virInterfaceDefPtr def) switch (def->type) { case VIR_INTERFACE_TYPE_BRIDGE: for (i = 0;i < def->data.bridge.nbItf;i++) { - if (def->data.bridge.itf[i] != NULL) - virInterfaceDefFree(def->data.bridge.itf[i]); - else + if (def->data.bridge.itf[i] == NULL) break; /* to cope with half parsed data on errors */ + virInterfaceDefFree(def->data.bridge.itf[i]); } VIR_FREE(def->data.bridge.itf); break; case VIR_INTERFACE_TYPE_BOND: VIR_FREE(def->data.bond.target); for (i = 0;i < def->data.bond.nbItf;i++) { - if (def->data.bond.itf[i] != NULL) - virInterfaceDefFree(def->data.bond.itf[i]); - else + if (def->data.bond.itf[i] == NULL) break; /* to cope with half parsed data on errors */ + virInterfaceDefFree(def->data.bond.itf[i]); } VIR_FREE(def->data.bond.itf); break; @@ -1227,8 +1225,7 @@ virInterfaceObjPtr virInterfaceAssignDef(virInterfaceObjListPtr interfaces, virInterfaceObjPtr iface; if ((iface = virInterfaceFindByName(interfaces, def->name))) { - if (iface->def) - virInterfaceDefFree(iface->def); + virInterfaceDefFree(iface->def); iface->def = def; return iface; diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 1f3a44c..06537d1 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -157,8 +157,7 @@ virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkDefFree(network->def); network->def = def; } else { - if (network->newDef) - virNetworkDefFree(network->newDef); + virNetworkDefFree(network->newDef); network->newDef = def; } diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 6218e02..778b909 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1332,8 +1332,7 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, virStoragePoolDefFree(pool->def); pool->def = def; } else { - if (pool->newDef) - virStoragePoolDefFree(pool->newDef); + virStoragePoolDefFree(pool->newDef); pool->newDef = def; } return pool; diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 0174794..8113c55 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -486,8 +486,7 @@ static void dev_create(const char *udi) DEBUG("FAILED TO ADD dev %s", name); cleanup: VIR_FREE(privData); - if (def) - virNodeDeviceDefFree(def); + virNodeDeviceDefFree(def); nodeDeviceUnlock(driverState); } diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index ae923e8..88cdc9d 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -63,8 +63,7 @@ vahDeinit(vahControl * ctl) return -1; VIR_FREE(ctl->def); - if (ctl->caps) - virCapabilitiesFree(ctl->caps); + virCapabilitiesFree(ctl->caps); VIR_FREE(ctl->files); VIR_FREE(ctl->hvm); VIR_FREE(ctl->arch); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6706cba..395c8c9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4983,8 +4983,7 @@ testNodeDeviceCreateXML(virConnectPtr conn, def = NULL; cleanup: testDriverUnlock(driver); - if (def) - virNodeDeviceDefFree(def); + virNodeDeviceDefFree(def); VIR_FREE(wwnn); VIR_FREE(wwpn); return dev; diff --git a/src/util/conf.c b/src/util/conf.c index 38eb163..8682f7b 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -902,9 +902,7 @@ virConfSetValue (virConfPtr conf, conf->entries = cur; } } else { - if (cur->value) { - virConfFreeValue(cur->value); - } + virConfFreeValue(cur->value); cur->value = value; } return (0); -- 1.7.1.250.g7d1e8
On 05/17/2010 02:51 PM, Jim Meyering wrote:
Is this something that the useless-if-before-free test could have caught, rather than waiting for coverity?
Very good idea. Here's a patch to do that. Here's a complete patch:
From 59d7134bc925de86b807262896a76256b889c96a Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 17 May 2010 22:38:59 +0200 Subject: [PATCH] maint: add more free-like functions to the list and deal with fallout
* cfg.mk (useless_free_options): Add many vir*Free* function names, and then remove the useless if-before-free tests exposed by running make syntax-check.
Looks sane.
+useless_free_options = \ + --name=VIR_FREE \ ...
+# The following template was generated by this command: +# make ID && aid free|grep '^vi'|sed 's/ .*//;s/^/# /' +# N virBufferFreeAndReset +# y virCPUDefFree
Especially nice, along with the documentation of potential future cleanup projects. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/17/2010 02:51 PM, Jim Meyering wrote:
Is this something that the useless-if-before-free test could have caught, rather than waiting for coverity?
Very good idea. Here's a patch to do that. Here's a complete patch:
From 59d7134bc925de86b807262896a76256b889c96a Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 17 May 2010 22:38:59 +0200 Subject: [PATCH] maint: add more free-like functions to the list and deal with fallout
* cfg.mk (useless_free_options): Add many vir*Free* function names, and then remove the useless if-before-free tests exposed by running make syntax-check.
Looks sane.
+useless_free_options = \ + --name=VIR_FREE \ ...
+# The following template was generated by this command: +# make ID && aid free|grep '^vi'|sed 's/ .*//;s/^/# /' +# N virBufferFreeAndReset +# y virCPUDefFree
Especially nice, along with the documentation of potential future cleanup projects.
ACK.
Thanks for the quick review. I'll push things tomorrow.
participants (2)
-
Eric Blake -
Jim Meyering