1:37 p.m.
While running domainsnapshotxml2xmltest, it was found that valgrind pointed out
the following memory leaks:
==32176== 42 (32 direct, 10 indirect) bytes in 1 blocks are definitely lost in loss record
42 of 66
==32176== at 0x4A069EE: malloc (vg_replace_malloc.c:270)
==32176== by 0x4A06B62: realloc (vg_replace_malloc.c:662)
==32176== by 0x4C65A07: virReallocN (viralloc.c:243)
==32176== by 0x4C65B2E: virExpandN (viralloc.c:292)
==32176== by 0x4C65E30: virInsertElementsN (viralloc.c:434)
==32176== by 0x4CD71F3: virDomainDiskSourceDefParse (domain_conf.c:5078)
==32176== by 0x4CF6EF4: virDomainSnapshotDefParseNode (snapshot_conf.c:151)
==32176== by 0x4CF7314: virDomainSnapshotDefParseString (snapshot_conf.c:410)
==32176== by 0x41FB8D: testCompareXMLToXMLHelper (domainsnapshotxml2xmltest.c:100)
==32176== by 0x420FD1: virtTestRun (testutils.c:199)
==32176== by 0x41F859: mymain (domainsnapshotxml2xmltest.c:222)
==32176== by 0x42174D: virtTestMain (testutils.c:782)
==32176==
==32176== 128 (96 direct, 32 indirect) bytes in 1 blocks are definitely lost in loss
record 51 of 66
==32176== at 0x4A06BE0: realloc (vg_replace_malloc.c:662)
==32176== by 0x4C65A07: virReallocN (viralloc.c:243)
==32176== by 0x4C65B2E: virExpandN (viralloc.c:292)
==32176== by 0x4C65E30: virInsertElementsN (viralloc.c:434)
==32176== by 0x4CD71F3: virDomainDiskSourceDefParse (domain_conf.c:5078)
==32176== by 0x4CF6EF4: virDomainSnapshotDefParseNode (snapshot_conf.c:151)
==32176== by 0x4CF7314: virDomainSnapshotDefParseString (snapshot_conf.c:410)
==32176== by 0x41FB8D: testCompareXMLToXMLHelper (domainsnapshotxml2xmltest.c:100)
==32176== by 0x420FD1: virtTestRun (testutils.c:199)
==32176== by 0x41F859: mymain (domainsnapshotxml2xmltest.c:222)
==32176== by 0x42174D: virtTestMain (testutils.c:782)
==32176== by 0x3E6CE1ED1C: (below main) (libc-start.c:226)
==32176==
---
src/conf/snapshot_conf.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 12b0930..475525f 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
{
VIR_FREE(disk->name);
VIR_FREE(disk->file);
+ virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
}
void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
--
1.7.1
5:43 a.m.
On 02/22/2014 08:37 PM, Nehal J Wani wrote:
While running domainsnapshotxml2xmltest, it was found that valgrind
pointed out
the following memory leaks:
==32176== 42 (32 direct, 10 indirect) bytes in 1 blocks are definitely lost in loss
record 42 of 66
==32176== at 0x4A069EE: malloc (vg_replace_malloc.c:270)
==32176== by 0x4A06B62: realloc (vg_replace_malloc.c:662)
==32176== by 0x4C65A07: virReallocN (viralloc.c:243)
==32176== by 0x4C65B2E: virExpandN (viralloc.c:292)
==32176== by 0x4C65E30: virInsertElementsN (viralloc.c:434)
==32176== by 0x4CD71F3: virDomainDiskSourceDefParse (domain_conf.c:5078)
==32176== by 0x4CF6EF4: virDomainSnapshotDefParseNode (snapshot_conf.c:151)
==32176== by 0x4CF7314: virDomainSnapshotDefParseString (snapshot_conf.c:410)
==32176== by 0x41FB8D: testCompareXMLToXMLHelper (domainsnapshotxml2xmltest.c:100)
==32176== by 0x420FD1: virtTestRun (testutils.c:199)
==32176== by 0x41F859: mymain (domainsnapshotxml2xmltest.c:222)
==32176== by 0x42174D: virtTestMain (testutils.c:782)
==32176==
==32176== 128 (96 direct, 32 indirect) bytes in 1 blocks are definitely lost in loss
record 51 of 66
==32176== at 0x4A06BE0: realloc (vg_replace_malloc.c:662)
==32176== by 0x4C65A07: virReallocN (viralloc.c:243)
==32176== by 0x4C65B2E: virExpandN (viralloc.c:292)
==32176== by 0x4C65E30: virInsertElementsN (viralloc.c:434)
==32176== by 0x4CD71F3: virDomainDiskSourceDefParse (domain_conf.c:5078)
==32176== by 0x4CF6EF4: virDomainSnapshotDefParseNode (snapshot_conf.c:151)
==32176== by 0x4CF7314: virDomainSnapshotDefParseString (snapshot_conf.c:410)
==32176== by 0x41FB8D: testCompareXMLToXMLHelper (domainsnapshotxml2xmltest.c:100)
==32176== by 0x420FD1: virtTestRun (testutils.c:199)
==32176== by 0x41F859: mymain (domainsnapshotxml2xmltest.c:222)
==32176== by 0x42174D: virtTestMain (testutils.c:782)
==32176== by 0x3E6CE1ED1C: (below main) (libc-start.c:226)
==32176==
---
src/conf/snapshot_conf.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 12b0930..475525f 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
{
VIR_FREE(disk->name);
VIR_FREE(disk->file);
+ virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
This leaves nhosts and hosts at their original values, which seems to be OK
everywhere this function is called, but it might be a problem if someone tries
to reuse the disk definition instead of freeing it.
I'd rather write this as:
while (disk->nhosts)
virDomainDiskHostDefClear(&disk->hosts[--def->nhosts])
VIR_FREE(disk->hosts)
Jan
2:04 p.m.
> ---
> src/conf/snapshot_conf.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 12b0930..475525f 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
> {
> VIR_FREE(disk->name);
> VIR_FREE(disk->file);
> + virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
This leaves nhosts and hosts at their original values, which seems to be OK
everywhere this function is called, but it might be a problem if someone tries
to reuse the disk definition instead of freeing it.
I'd rather write this as:
while (disk->nhosts)
virDomainDiskHostDefClear(&disk->hosts[--def->nhosts])
VIR_FREE(disk->hosts)
Jan
1. Since virDomainDiskHostDefFree() already calls VIR_FREE(hosts),
shouldn't the modified patch just be:
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 12b0930..a233e8e 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -82,6 +82,8 @@
virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
{
VIR_FREE(disk->name);
VIR_FREE(disk->file);
+ virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
+ disk->nhosts = 0;
}
void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
2. I have a question. Why don't we have disk->nhosts = 0; each time
someone calls virDomainDiskHostDefFree(disk->nhosts, disk->hosts) in
./src/conf/domain_conf.c, ./src/storage/storage_driver.c and
./src/qemu/qemu_conf.c?
Or we can change the definition of virDomainDiskHostDefFree to
virDomainDiskHostDefFree(size_t *nhosts, virDomainDiskHostDefPtr hosts)
and make *nhosts = 0 inside the function itself and make the necessary
changes wherever this function is called?
--
Nehal J Wani
3:44 a.m.
[cc-ing Peter as he invented the function]
On 03/03/2014 09:04 PM, Nehal J Wani wrote:
>> ---
>> src/conf/snapshot_conf.c | 1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>> index 12b0930..475525f 100644
>> --- a/src/conf/snapshot_conf.c
>> +++ b/src/conf/snapshot_conf.c
>> @@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr
disk)
>> {
>> VIR_FREE(disk->name);
>> VIR_FREE(disk->file);
>> + virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
>
> This leaves nhosts and hosts at their original values, which seems to be OK
> everywhere this function is called, but it might be a problem if someone tries
> to reuse the disk definition instead of freeing it.
>
> I'd rather write this as:
>
> while (disk->nhosts)
> virDomainDiskHostDefClear(&disk->hosts[--def->nhosts])
> VIR_FREE(disk->hosts)
>
> Jan
>
1. Since virDomainDiskHostDefFree() already calls VIR_FREE(hosts),
shouldn't the modified patch just be:
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 12b0930..a233e8e 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -82,6 +82,8 @@
virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
{
VIR_FREE(disk->name);
VIR_FREE(disk->file);
+ virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
+ disk->nhosts = 0;
}
This leaves the pointer to the freed memory in disk->hosts.
void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
2. I have a question. Why don't we have disk->nhosts = 0; each time
someone calls virDomainDiskHostDefFree(disk->nhosts, disk->hosts) in
./src/conf/domain_conf.c, ./src/storage/storage_driver.c and
./src/qemu/qemu_conf.c?
In most cases either the whole def gets freed, or hosts and nhosts are
overwritten. I'm not sure if we overwrite it in all cases in
qemuTranslateDiskSourcePool or it needs to be added there too.
Or we can change the definition of virDomainDiskHostDefFree to
virDomainDiskHostDefFree(size_t *nhosts, virDomainDiskHostDefPtr hosts)
and make *nhosts = 0 inside the function itself and make the necessary
changes wherever this function is called?
That looks more awkward than the original function to me.
I think it should be called virDomainDiskHostDefsFree instead, since it's
different from all the other DefFree functions, which only take one DefPtr
argument. The callers should make sure that the leftover values in
nhosts/hosts aren't reused.
Jan
9:58 a.m.
On 03/04/14 10:44, Ján Tomko wrote:
[cc-ing Peter as he invented the function]
On 03/03/2014 09:04 PM, Nehal J Wani wrote:
>>> ---
>>> src/conf/snapshot_conf.c | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>>> index 12b0930..475525f 100644
>>> --- a/src/conf/snapshot_conf.c
>>> +++ b/src/conf/snapshot_conf.c
>>> @@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr
disk)
>>> {
>>> VIR_FREE(disk->name);
>>> VIR_FREE(disk->file);
>>> + virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
>>
>> This leaves nhosts and hosts at their original values, which seems to be OK
>> everywhere this function is called, but it might be a problem if someone tries
>> to reuse the disk definition instead of freeing it.
Maybe we should just note that this is happening in the function and let
our future selves worry about that in case somebody will need to use the
clear function in a different context.
>>
>> I'd rather write this as:
>>
>> while (disk->nhosts)
>> virDomainDiskHostDefClear(&disk->hosts[--def->nhosts])
>> VIR_FREE(disk->hosts)
>>
>> Jan
>>
>
> 1. Since virDomainDiskHostDefFree() already calls VIR_FREE(hosts),
> shouldn't the modified patch just be:
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 12b0930..a233e8e 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -82,6 +82,8 @@
> virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
> {
> VIR_FREE(disk->name);
> VIR_FREE(disk->file);
> + virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
> + disk->nhosts = 0;
> }
This leaves the pointer to the freed memory in disk->hosts.
>
> void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
>
> 2. I have a question. Why don't we have disk->nhosts = 0; each time
> someone calls virDomainDiskHostDefFree(disk->nhosts, disk->hosts) in
> ./src/conf/domain_conf.c, ./src/storage/storage_driver.c and
> ./src/qemu/qemu_conf.c?
In most cases either the whole def gets freed, or hosts and nhosts are
overwritten. I'm not sure if we overwrite it in all cases in
qemuTranslateDiskSourcePool or it needs to be added there too.
> Or we can change the definition of virDomainDiskHostDefFree to
> virDomainDiskHostDefFree(size_t *nhosts, virDomainDiskHostDefPtr hosts)
> and make *nhosts = 0 inside the function itself and make the necessary
> changes wherever this function is called?
>
That looks more awkward than the original function to me.
Seconded. Additionally it would require a macro as calling the function
with reference operators would be even more ugly.
I think it should be called virDomainDiskHostDefsFree instead, since it's
different from all the other DefFree functions, which only take one DefPtr
argument. The callers should make sure that the leftover values in
nhosts/hosts aren't reused.
Hmmm I must agree that my choice of name for this function wasn't ideal.
It does indeed free more entries, so that the plural form would be a
better choice.
Jan
Peter
3946
days inactive
3956
days old
4 comments
3 participants
participants (3)
-
Ján Tomko -
Nehal J Wani -
Peter Krempa