[libvirt] PATCH: Fix some more memory leaks

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> 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.