Re: [libvirt] [PATCH] Fix memory leak in virDomainSnapshotDiskDefClear()

2014-03-04 Thread Ján Tomko 
[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



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fix memory leak in virDomainSnapshotDiskDefClear()

2014-03-04 Thread Peter Krempa 
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



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fix memory leak in virDomainSnapshotDiskDefClear()

2014-03-03 Thread Nehal J Wani 
 ---
  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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fix memory leak in virDomainSnapshotDiskDefClear()

2014-02-26 Thread Ján Tomko 
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



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Fix memory leak in virDomainSnapshotDiskDefClear()

2014-02-22 Thread Nehal J Wani 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list