[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 :|
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/
participants (2)
-
Daniel P. Berrange -
Richard W.M. Jones