Re: [libvirt] [PATCH] daemon: Plug memory leaks

2012-04-12 Thread Alex Jia 

On 04/12/2012 04:37 PM, Daniel P. Berrange wrote:

On Thu, Apr 12, 2012 at 03:24:08PM +0800, Alex Jia wrote:

Detected by valgrind. Leaks are introduced in commit 6e6e9be.

* daemon/libvirtd-config.c (macro GET_CONF_STR): fix memory leaks.

How to reproduce?

% make&&  make -C tests check TESTS=libvirtdconftest
% cd tests&&  valgrind -v --leak-check=full ./libvirtdconftest

actual result:

==11008== 185 bytes in 5 blocks are definitely lost in loss record 3 of 5
==11008==at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==11008==by 0x39CF07F6E1: strdup (strdup.c:43)
==11008==by 0x406626: daemonConfigLoadOptions (libvirtd-config.c:438)
==11008==by 0x406800: daemonConfigLoadData (libvirtd-config.c:492)
==11008==by 0x403CCF: testCorrupt (libvirtdconftest.c:110)
==11008==by 0x404FAD: virtTestRun (testutils.c:145)
==11008==by 0x403A34: mymain (libvirtdconftest.c:219)
==11008==by 0x404687: virtTestMain (testutils.c:700)
==11008==by 0x39CF01ECDC: (below main) (libc-start.c:226)
==11008==
==11008== LEAK SUMMARY:
==11008==definitely lost: 185 bytes in 5 blocks

Signed-off-by: Alex Jia
---
  daemon/libvirtd-config.c |1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index 4d041f0..0a4323a 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -152,6 +152,7 @@ checkType (virConfValuePtr p, const char *filename,
  virReportOOMError();\
  goto error; \
  }   \
+VIR_FREE(data->var_name);   \
  }   \
  } while (0)

Errr, no. Please look at the context of what's going on, rather than
just blindly adding code to free a block at the point where valgrind
shows the allocation.

The full context to this diff

#define GET_CONF_STR(conf, filename, var_name)  \
 do {\
 virConfValuePtr p = virConfGetValue (conf, #var_name);  \
 if (p) {\
 if (checkType (p, filename, #var_name, VIR_CONF_STRING)<  0) \
 goto error; \
 VIR_FREE(data->var_name);   \
 if (!(data->var_name = strdup (p->str))) {  \
 virReportOOMError();\
 goto error; \
 }   \
 }   \
 } while (0)


Your suggested change means we would be free'ing the config file before
we have even used it.

If you look at this line:

Yeah.

==11008==by 0x406626: daemonConfigLoadOptions (libvirtd-config.c:438)

You'll see it matches:

 GET_CONF_STR (conf, filename, host_uuid);

There are no leak reports for other uses of GET_CONF_STR, so this suggests
that 'host_uuid' is not being freed. Check daemonConfigFree and you'll see
this is indeed missing
Yeah, it's a little weird, the previous codes also haven't free 
'host_uuid', but valgrind hasn't complains memory leak.


Thanks for your review,
Alex

Daniel


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

Re: [libvirt] [PATCH] daemon: Plug memory leaks

2012-04-12 Thread Daniel P. Berrange 
On Thu, Apr 12, 2012 at 03:24:08PM +0800, Alex Jia wrote:
> Detected by valgrind. Leaks are introduced in commit 6e6e9be.
> 
> * daemon/libvirtd-config.c (macro GET_CONF_STR): fix memory leaks.
> 
> How to reproduce?
> 
> % make && make -C tests check TESTS=libvirtdconftest
> % cd tests && valgrind -v --leak-check=full ./libvirtdconftest
> 
> actual result:
> 
> ==11008== 185 bytes in 5 blocks are definitely lost in loss record 3 of 5
> ==11008==at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
> ==11008==by 0x39CF07F6E1: strdup (strdup.c:43)
> ==11008==by 0x406626: daemonConfigLoadOptions (libvirtd-config.c:438)
> ==11008==by 0x406800: daemonConfigLoadData (libvirtd-config.c:492)
> ==11008==by 0x403CCF: testCorrupt (libvirtdconftest.c:110)
> ==11008==by 0x404FAD: virtTestRun (testutils.c:145)
> ==11008==by 0x403A34: mymain (libvirtdconftest.c:219)
> ==11008==by 0x404687: virtTestMain (testutils.c:700)
> ==11008==by 0x39CF01ECDC: (below main) (libc-start.c:226)
> ==11008==
> ==11008== LEAK SUMMARY:
> ==11008==definitely lost: 185 bytes in 5 blocks
> 
> Signed-off-by: Alex Jia 
> ---
>  daemon/libvirtd-config.c |1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
> index 4d041f0..0a4323a 100644
> --- a/daemon/libvirtd-config.c
> +++ b/daemon/libvirtd-config.c
> @@ -152,6 +152,7 @@ checkType (virConfValuePtr p, const char *filename,
>  virReportOOMError();\
>  goto error; \
>  }   \
> +VIR_FREE(data->var_name);   \
>  }   \
>  } while (0)

Errr, no. Please look at the context of what's going on, rather than
just blindly adding code to free a block at the point where valgrind
shows the allocation.

The full context to this diff

#define GET_CONF_STR(conf, filename, var_name)  \
do {\
virConfValuePtr p = virConfGetValue (conf, #var_name);  \
if (p) {\
if (checkType (p, filename, #var_name, VIR_CONF_STRING) < 0) \
goto error; \
VIR_FREE(data->var_name);   \
if (!(data->var_name = strdup (p->str))) {  \
virReportOOMError();\
goto error; \
}   \
}   \
} while (0)


Your suggested change means we would be free'ing the config file before
we have even used it.

If you look at this line:

> ==11008==by 0x406626: daemonConfigLoadOptions (libvirtd-config.c:438)

You'll see it matches:

GET_CONF_STR (conf, filename, host_uuid);

There are no leak reports for other uses of GET_CONF_STR, so this suggests
that 'host_uuid' is not being freed. Check daemonConfigFree and you'll see
this is indeed missing

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

[libvirt] [PATCH] daemon: Plug memory leaks

2012-04-12 Thread Alex Jia 
Detected by valgrind. Leaks are introduced in commit 6e6e9be.

* daemon/libvirtd-config.c (macro GET_CONF_STR): fix memory leaks.

How to reproduce?

% make && make -C tests check TESTS=libvirtdconftest
% cd tests && valgrind -v --leak-check=full ./libvirtdconftest

actual result:

==11008== 185 bytes in 5 blocks are definitely lost in loss record 3 of 5
==11008==at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==11008==by 0x39CF07F6E1: strdup (strdup.c:43)
==11008==by 0x406626: daemonConfigLoadOptions (libvirtd-config.c:438)
==11008==by 0x406800: daemonConfigLoadData (libvirtd-config.c:492)
==11008==by 0x403CCF: testCorrupt (libvirtdconftest.c:110)
==11008==by 0x404FAD: virtTestRun (testutils.c:145)
==11008==by 0x403A34: mymain (libvirtdconftest.c:219)
==11008==by 0x404687: virtTestMain (testutils.c:700)
==11008==by 0x39CF01ECDC: (below main) (libc-start.c:226)
==11008==
==11008== LEAK SUMMARY:
==11008==definitely lost: 185 bytes in 5 blocks

Signed-off-by: Alex Jia 
---
 daemon/libvirtd-config.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index 4d041f0..0a4323a 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -152,6 +152,7 @@ checkType (virConfValuePtr p, const char *filename,
 virReportOOMError();\
 goto error; \
 }   \
+VIR_FREE(data->var_name);   \
 }   \
 } while (0)
 
-- 
1.7.1

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