12:22 p.m.
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(a)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
12:34 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:40 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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(a)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
12:49 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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.
12:53 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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);
}
12:57 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:04 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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(a)gnu.org
1:11 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:51 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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(a)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
3:55 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4 p.m.
New subject: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
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(a)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.
5304
days inactive
5304
days old
11 comments
2 participants
participants (2)
-
Eric Blake -
Jim Meyering