[Libguestfs] [PATCH 1/2] lib: Add CLEANUP_FREE macro which automatically calls 'free' when leaving scope.

[Richard W.M. Jones]

On Thu, Jan 24, 2013 at 05:41:17PM +0000, Daniel P. Berrange wrote:
> On Thu, Jan 24, 2013 at 05:13:11PM +0000, Richard W.M. Jones wrote:
> > From: "Richard W.M. Jones" <rjones at redhat.com>
> > 
> > Use the macro like this to create temporary variables which are
> > automatically cleaned up when the scope is exited:
> > 
> >   {
> >     CLEANUP_FREE (char *, foo, strdup (bar)); /* char *foo = strdup (bar) */
> >     ...
> >     // no need to call free (foo)!
> >   }
> > 
> > On GCC and LLVM, this is implemented using __attribute__((cleanup(...))).
> > 
> > On other compilers, we fall back to a less efficient implementation
> > which saves up the memory allocations and frees them all when the
> > handle is closed.
> > ---
> >  configure.ac           | 35 +++++++++++++++++++++++++++++++++++
> >  src/alloc.c            | 19 +++++++++++++++++++
> >  src/guestfs-internal.h | 33 +++++++++++++++++++++++++++++++++
> >  src/handle.c           | 15 +++++++++++++++
> >  4 files changed, 102 insertions(+)
> > 
> 
> > +#ifndef HAVE_ATTRIBUTE_CLEANUP
> > +void
> > +guestfs___defer_free (guestfs_h *g, void (*freefn) (void *), void *data)
> > +{
> > +  struct deferred_free *p = safe_malloc (g, sizeof *p);
> > +
> > +  p->freefn = freefn;
> > +  p->data = data;
> > +  p->next = g->deferred_frees;
> > +  g->deferred_frees = p;
> > +}
> > +#endif
> 
> ...
> 
> >  guestfs_close (guestfs_h *g)
> >  {
> >    struct qemu_param *qp, *qp_next;
> > +#ifndef HAVE_ATTRIBUTE_CLEANUP
> > +  struct deferred_free *dfp, *dfp_next;
> > +#endif
> >    guestfs_h **gg;
> >  
> >    if (g->state == NO_HANDLE) {
> > @@ -324,6 +327,18 @@ guestfs_close (guestfs_h *g)
> >    while (g->error_cb_stack)
> >      guestfs_pop_error_handler (g);
> >  
> > +#ifndef HAVE_ATTRIBUTE_CLEANUP
> > +  /* For compilers that don't support __attribute__((cleanup(...))),
> > +   * free any temporary data that we allocated in CLEANUP_* macros
> > +   * here.
> > +   */
> > +  for (dfp = g->deferred_frees; dfp; dfp = dfp_next) {
> > +    dfp->freefn (&dfp->data);
> > +    dfp_next = dfp->next;
> > +    free (dfp);
> > +  }
> > +#endif
> > +
> 
> So if I'm understanding correctly, in the fallback case, this means that
> no memory is free'd until guest_close() is invoked. This may work if you
> have a fairly short lived  guestfs_h * handle, but if the application
> opens a handle and uses it for alot of operations for a very long time
> without closing, I think this design is effectively a memory leak, or
> least leads to ever increasing memory usage for a long time.

We could call the cleanup more frequently, eg. on entry to any
guestfs_* function.

The larger problem was pointed out to me in private mail is that the
fallback case doesn't work.  For example:

  CLEANUP_FREE (char *, p, malloc (100));

  p = realloc (p, 200);

This works fine in the GCC/LLVM case.  In the fallback case it could
cause a double free.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/