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

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(+)

diff --git a/configure.ac b/configure.ac
index 39c79cf..7745ab5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -191,6 +191,41 @@ AC_SYS_LARGEFILE
 dnl Check sizeof long.
 AC_CHECK_SIZEOF([long])
 
+dnl Check if __attribute__((cleanup(...))) works.
+dnl XXX It would be nice to use AC_COMPILE_IFELSE here, but gcc just
+dnl emits a warning for attributes that it doesn't understand.
+AC_MSG_CHECKING([if __attribute__((cleanup(...))) works with this compiler])
+AC_RUN_IFELSE([
+AC_LANG_SOURCE([[
+#include <stdio.h>
+#include <stdlib.h>
+
+void
+freep (void *ptr)
+{
+  exit (0);
+}
+
+void
+test (void)
+{
+  __attribute__((cleanup(freep))) char *ptr = malloc (100);
+}
+
+int
+main (int argc, char *argv[])
+{
+  test ();
+  exit (1);
+}
+]])
+    ],[
+    AC_MSG_RESULT([yes])
+    AC_DEFINE([HAVE_ATTRIBUTE_CLEANUP],[1],[Define to 1 if
'__attribute__((cleanup(...)))' works with this compiler.])
+    ],[
+    AC_MSG_RESULT([no])
+    ])
+
 dnl Check if dirent (readdir) supports d_type member.
 AC_STRUCT_DIRENT_D_TYPE
 
diff --git a/src/alloc.c b/src/alloc.c
index 25d7f42..cf3741a 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -122,3 +122,22 @@ guestfs___safe_asprintf (guestfs_h *g, const char *fs, ...)
 
   return msg;
 }
+
+void
+guestfs___cleanup_free (void *ptr)
+{
+  free (* (void **) ptr);
+}
+
+#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
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 870207b..d27a3c2 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -72,6 +72,19 @@
 #define TRACE4(name, arg1, arg2, arg3, arg4)
 #endif
 
+#ifdef HAVE_ATTRIBUTE_CLEANUP
+#define CLEANUP(type, var, init, freefn)                \
+  __attribute__((cleanup(freefn))) type var = init
+#else
+#define CLEANUP(type, var, init, freefn)                \
+  type var = init;                                      \
+  guestfs___defer_free ((g), (freefn), (var))
+#endif
+
+#define CLEANUP_FREE(type, var, init)                   \
+  CLEANUP(type, var, (init), guestfs___cleanup_free)
+/* other CLEANUP_* macros to follow */
+
 #define TMP_TEMPLATE_ON_STACK(g,var)                      \
   char *ttos_tmpdir = guestfs_get_tmpdir (g);             \
   char var[strlen (ttos_tmpdir) + 32];                    \
@@ -324,6 +337,10 @@ struct guestfs_h
   virConnectCredentialPtr requested_credentials;
 #endif
 
+#ifndef HAVE_ATTRIBUTE_CLEANUP
+  struct deferred_free *deferred_frees;
+#endif
+
   /**** Private data for attach-methods. ****/
   /* NB: This cannot be a union because of a pathological case where
    * the user changes attach-method while reusing the handle to launch
@@ -356,6 +373,14 @@ struct guestfs_h
 #endif
 };
 
+#ifndef HAVE_ATTRIBUTE_CLEANUP
+struct deferred_free {
+  struct deferred_free *next;
+  void (*freefn) (void *);
+  void *data;
+};
+#endif
+
 /* Per-filesystem data stored for inspect_os. */
 enum inspect_os_format {
   OS_FORMAT_UNKNOWN = 0,
@@ -474,6 +499,14 @@ extern char *guestfs___safe_asprintf (guestfs_h *g, const
char *fs, ...)
 #define safe_memdup guestfs___safe_memdup
 #define safe_asprintf guestfs___safe_asprintf
 
+/* These functions are used internally by the CLEANUP_* macros.
+ * Don't call them directly.
+ */
+extern void guestfs___cleanup_free (void *ptr);
+#ifndef HAVE_ATTRIBUTE_CLEANUP
+extern void guestfs___defer_free (guestfs_h *g, void (*freefn) (void *), void
*data);
+#endif
+
 /* errors.c */
 extern void guestfs___init_error_handler (guestfs_h *g);
 
diff --git a/src/handle.c b/src/handle.c
index 39e30c7..8cf7a40 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -253,6 +253,9 @@ void
 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
+
   if (g->pda)
     hash_free (g->pda);
   free (g->tmpdir);
-- 
1.8.1

From: "Richard W.M. Jones" <rjones@redhat.com>

---
 src/inspect-fs.c | 17 ++++-------------
 src/inspect.c    |  8 ++------
 2 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/src/inspect-fs.c b/src/inspect-fs.c
index fed7761..724d4ee 100644
--- a/src/inspect-fs.c
+++ b/src/inspect-fs.c
@@ -89,7 +89,6 @@ int
 guestfs___check_for_filesystem_on (guestfs_h *g, const char *device,
                                    int is_block, int is_partnum)
 {
-  char *vfs_type;
   int is_swap, r;
   struct inspect_fs *fs;
 
@@ -98,7 +97,7 @@ guestfs___check_for_filesystem_on (guestfs_h *g, const char
*device,
    * temporarily replace the error handler with a null one.
    */
   guestfs_push_error_handler (g, NULL, NULL);
-  vfs_type = guestfs_vfs_type (g, device);
+  CLEANUP_FREE (char *, vfs_type, guestfs_vfs_type (g, device));
   guestfs_pop_error_handler (g);
 
   is_swap = vfs_type && STREQ (vfs_type, "swap");
@@ -108,7 +107,6 @@ guestfs___check_for_filesystem_on (guestfs_h *g, const char
*device,
          vfs_type ? vfs_type : "failed to get vfs type");
 
   if (is_swap) {
-    free (vfs_type);
     if (extend_fses (g) == -1)
       return -1;
     fs = &g->fses[g->nr_fses-1];
@@ -145,7 +143,6 @@ guestfs___check_for_filesystem_on (guestfs_h *g, const char
*device,
   } else {
     r = guestfs_mount_ro (g, device, "/");
   }
-  free (vfs_type);
   guestfs_pop_error_handler (g);
   if (r == -1)
     return 0;
@@ -341,28 +338,24 @@ extend_fses (guestfs_h *g)
 int
 guestfs___is_file_nocase (guestfs_h *g, const char *path)
 {
-  char *p;
   int r;
 
-  p = guestfs___case_sensitive_path_silently (g, path);
+  CLEANUP_FREE (char *, p, guestfs___case_sensitive_path_silently (g, path));
   if (!p)
     return 0;
   r = guestfs_is_file (g, p);
-  free (p);
   return r > 0;
 }
 
 int
 guestfs___is_dir_nocase (guestfs_h *g, const char *path)
 {
-  char *p;
   int r;
 
-  p = guestfs___case_sensitive_path_silently (g, path);
+  CLEANUP_FREE (char *, p, guestfs___case_sensitive_path_silently (g, path));
   if (!p)
     return 0;
   r = guestfs_is_dir (g, p);
-  free (p);
   return r > 0;
 }
 
@@ -532,7 +525,6 @@ guestfs___check_package_management (guestfs_h *g, struct
inspect_fs *fs)
 char *
 guestfs___first_line_of_file (guestfs_h *g, const char *filename)
 {
-  char **lines;
   int64_t size;
   char *ret;
 
@@ -549,7 +541,7 @@ guestfs___first_line_of_file (guestfs_h *g, const char
*filename)
     return NULL;
   }
 
-  lines = guestfs_head_n (g, 1, filename);
+  CLEANUP_FREE (char **, lines, guestfs_head_n (g, 1, filename));
   if (lines == NULL)
     return NULL;
   if (lines[0] == NULL) {
@@ -560,7 +552,6 @@ guestfs___first_line_of_file (guestfs_h *g, const char
*filename)
   /* lines[1] should be NULL because of '1' argument above ... */
 
   ret = lines[0];               /* caller frees */
-  free (lines);                 /* free the array */
 
   return ret;
 }
diff --git a/src/inspect.c b/src/inspect.c
index 30e9762..f165957 100644
--- a/src/inspect.c
+++ b/src/inspect.c
@@ -143,23 +143,19 @@ guestfs__inspect_os (guestfs_h *g)
 static int
 parent_device_already_probed (guestfs_h *g, const char *partition)
 {
-  char *device;
   size_t i;
 
   guestfs_push_error_handler (g, NULL, NULL);
-  device = guestfs_part_to_dev (g, partition);
+  CLEANUP_FREE (char *, device, guestfs_part_to_dev (g, partition));
   guestfs_pop_error_handler (g);
   if (!device)
     return 0;
 
   for (i = 0; i < g->nr_fses; ++i) {
-    if (STREQ (device, g->fses[i].device)) {
-      free (device);
+    if (STREQ (device, g->fses[i].device))
       return 1;
-    }
   }
 
-  free (device);
   return 0;
 }
 
-- 
1.8.1

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.

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 :|