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@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/