"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> While testing Cole's series of patches I identified a couple more places
> where we leak memory.
>
> In libvirt.c, the default authentication callback uses uninitialized
> data, and indeed strdup()'s it and this is then never released. This
> simply disables that bit of code.
>
> In qparams.c when free'ing the struct the 'p' struct field was not
> released. I took the opportunity to switch it over to using the new
> style memory.h functions
>
> In remote.c there were a couple of handlers which forgot to free the
> virDomainPtr object when they were done.
>
> qemud/remote.c | 18 ++++++++++++++----
> src/libvirt.c | 16 +++++++++-------
> src/qparams.c | 31 +++++++++++++------------------
> 3 files changed, 36 insertions(+), 29 deletions(-)
Looks good.
I suppose the tests that exposed the leaks aren't yet run
as part of "make check". It'd be nice...
> Index: qemud/remote.c
> ===================================================================
> RCS file: /data/cvs/libvirt/qemud/remote.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 remote.c
> --- qemud/remote.c 21 May 2008 20:53:30 -0000 1.32
> +++ qemud/remote.c 21 May 2008 21:16:42 -0000
> @@ -792,8 +792,11 @@ remoteDispatchDomainBlockStats (struct q
> }
> path = args->path;
>
> - if (virDomainBlockStats (dom, path, &stats, sizeof stats) == -1)
> + if (virDomainBlockStats (dom, path, &stats, sizeof stats) == -1) {
> + virDomainFree (dom);
> return -1;
> + }
> + virDomainFree (dom);
Recently, I've come to prefer using a single free call,
snuggled right up against the last use (as long as you don't
mind the additional variable). E.g.,
fail = (virDomainBlockStats (dom, path, &stats, sizeof stats) == -1);
virDomainFree (dom);
if (fail)
return -1;
> ret->rd_req = stats.rd_req;
> ret->rd_bytes = stats.rd_bytes;
> @@ -823,8 +826,11 @@ remoteDispatchDomainInterfaceStats (stru
> }
> path = args->path;
>
> - if (virDomainInterfaceStats (dom, path, &stats, sizeof stats) == -1)
> + if (virDomainInterfaceStats (dom, path, &stats, sizeof stats) == -1) {
> + virDomainFree (dom);
> return -1;
> + }
> + virDomainFree (dom);
>
> ret->rx_bytes = stats.rx_bytes;
> ret->rx_packets = stats.rx_packets;
> @@ -1258,7 +1264,11 @@ remoteDispatchDomainMigratePerform (stru
> args->cookie.cookie_len,
> args->uri,
> args->flags, dname, args->resource);
> - if (r == -1) return -1;
> + if (r == -1) {
> + virDomainFree (dom);
> + return -1;
> + }
> + virDomainFree (dom);
Here, you can call virDomainFree just once, before the "if", regardless.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list