6:55 a.m.
Acquire the per-handle lock on entering each public API function.
The lock is released by a cleanup handler, so we only need to use the
ACQUIRE_LOCK_FOR_CURRENT_SCOPE macro at the top of each function.
Note this means we require __attribute__((cleanup)). On platforms
where this is not supported, the code will probably hang whenever a
libguestfs function is called.
The only definitive list of public APIs is found indirectly in the
generator (in generator/c.ml : globals).
---
common/utils/cleanup.c | 10 +++++++++-
common/utils/guestfs-internal-frontend.h | 4 ++++
generator/c.ml | 4 ++++
lib/errors.c | 8 ++++++++
lib/events.c | 8 ++++++++
lib/guestfs-internal.h | 8 ++++++++
lib/handle.c | 17 ++++++++++++++++-
lib/private-data.c | 7 +++++++
8 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/common/utils/cleanup.c b/common/utils/cleanup.c
index 6c4558c39..b0b0362fd 100644
--- a/common/utils/cleanup.c
+++ b/common/utils/cleanup.c
@@ -1,5 +1,5 @@
/* libguestfs
- * Copyright (C) 2013 Red Hat Inc.
+ * Copyright (C) 2013-2017 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -68,6 +68,7 @@
#include <libxml/xpath.h>
#include <libxml/xmlwriter.h>
+#include "glthread/lock.h"
#include "hash.h"
#include "guestfs.h"
@@ -185,3 +186,10 @@ guestfs_int_cleanup_pclose (void *ptr)
if (f)
pclose (f);
}
+
+void
+guestfs_int_cleanup_gl_recursive_lock_unlock (void *ptr)
+{
+ gl_recursive_lock_t *lockp = * (gl_recursive_lock_t **) ptr;
+ gl_recursive_lock_unlock (*lockp);
+}
diff --git a/common/utils/guestfs-internal-frontend.h
b/common/utils/guestfs-internal-frontend.h
index e48f4eb49..0cdceafcc 100644
--- a/common/utils/guestfs-internal-frontend.h
+++ b/common/utils/guestfs-internal-frontend.h
@@ -63,6 +63,8 @@
__attribute__((cleanup(guestfs_int_cleanup_xmlXPathFreeObject)))
#define CLEANUP_FCLOSE __attribute__((cleanup(guestfs_int_cleanup_fclose)))
#define CLEANUP_PCLOSE __attribute__((cleanup(guestfs_int_cleanup_pclose)))
+#define CLEANUP_GL_RECURSIVE_LOCK_UNLOCK \
+ __attribute__((cleanup(guestfs_int_cleanup_gl_recursive_lock_unlock)))
#else
#define CLEANUP_FREE
#define CLEANUP_FREE_STRING_LIST
@@ -77,6 +79,7 @@
#define CLEANUP_XMLXPATHFREEOBJECT
#define CLEANUP_FCLOSE
#define CLEANUP_PCLOSE
+/* XXX no safe equivalent to CLEANUP_GL_RECURSIVE_LOCK_UNLOCK */
#endif
/* utils.c */
@@ -130,6 +133,7 @@ extern void guestfs_int_cleanup_xmlXPathFreeContext (void *ptr);
extern void guestfs_int_cleanup_xmlXPathFreeObject (void *ptr);
extern void guestfs_int_cleanup_fclose (void *ptr);
extern void guestfs_int_cleanup_pclose (void *ptr);
+extern void guestfs_int_cleanup_gl_recursive_lock_unlock (void *ptr);
/* These are in a separate header so the header can be generated.
* Don't include the following file directly:
diff --git a/generator/c.ml b/generator/c.ml
index 1f099a221..dd5f99559 100644
--- a/generator/c.ml
+++ b/generator/c.ml
@@ -1693,6 +1693,7 @@ and generate_client_actions actions () =
~dll_public:true
c_name style;
pr "{\n";
+ pr " ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);\n";
handle_null_optargs optargs c_name;
@@ -1779,6 +1780,7 @@ and generate_client_actions actions () =
c_name style;
pr "{\n";
+ pr " ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);\n";
handle_null_optargs optargs c_name;
@@ -2124,6 +2126,7 @@ and generate_client_actions_variants () =
~handle:"g" ~prefix:"guestfs_" ~suffix:"_va"
~optarg_proto:VA
c_name style;
pr "{\n";
+ pr " ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);\n";
pr " struct guestfs_%s_argv optargs_s;\n" c_name;
pr " struct guestfs_%s_argv *optargs = &optargs_s;\n" c_name;
pr " int i;\n";
@@ -2181,6 +2184,7 @@ and generate_client_actions_variants () =
~handle:"g" ~prefix:"guestfs_"
name (ret, args, []);
pr "{\n";
+ pr " ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);\n";
pr " struct guestfs_%s_opts_argv optargs_s = { .bitmask = 0 };\n" name;
pr " struct guestfs_%s_opts_argv *optargs = &optargs_s;\n" name;
pr "\n";
diff --git a/lib/errors.c b/lib/errors.c
index ace6a89cf..def1d3c89 100644
--- a/lib/errors.c
+++ b/lib/errors.c
@@ -54,12 +54,14 @@
const char *
guestfs_last_error (guestfs_h *g)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
return g->last_error;
}
int
guestfs_last_errno (guestfs_h *g)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
return g->last_errnum;
}
@@ -217,12 +219,14 @@ guestfs_int_perrorf (guestfs_h *g, const char *fs, ...)
void
guestfs_set_out_of_memory_handler (guestfs_h *g, guestfs_abort_cb cb)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
g->abort_cb = cb;
}
guestfs_abort_cb
guestfs_get_out_of_memory_handler (guestfs_h *g)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
return g->abort_cb;
}
@@ -230,6 +234,7 @@ void
guestfs_set_error_handler (guestfs_h *g,
guestfs_error_handler_cb cb, void *data)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
g->error_cb = cb;
g->error_cb_data = data;
}
@@ -237,6 +242,7 @@ guestfs_set_error_handler (guestfs_h *g,
guestfs_error_handler_cb
guestfs_get_error_handler (guestfs_h *g, void **data_rtn)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
if (data_rtn) *data_rtn = g->error_cb_data;
return g->error_cb;
}
@@ -245,6 +251,7 @@ void
guestfs_push_error_handler (guestfs_h *g,
guestfs_error_handler_cb cb, void *data)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
struct error_cb_stack *old_stack;
old_stack = g->error_cb_stack;
@@ -259,6 +266,7 @@ guestfs_push_error_handler (guestfs_h *g,
void
guestfs_pop_error_handler (guestfs_h *g)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
struct error_cb_stack *next_stack;
if (g->error_cb_stack) {
diff --git a/lib/events.c b/lib/events.c
index 1bddd7611..8005b1cc8 100644
--- a/lib/events.c
+++ b/lib/events.c
@@ -35,6 +35,7 @@ guestfs_set_event_callback (guestfs_h *g,
int flags,
void *opaque)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
int event_handle;
if (flags != 0) {
@@ -69,6 +70,8 @@ guestfs_set_event_callback (guestfs_h *g,
void
guestfs_delete_event_callback (guestfs_h *g, int event_handle)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
+
if (event_handle < 0 || event_handle >= (int) g->nr_events)
return;
@@ -296,6 +299,7 @@ void
guestfs_set_log_message_callback (guestfs_h *g,
guestfs_log_message_cb cb, void *opaque)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
replace_old_style_event_callback (g, log_message_callback_wrapper,
GUESTFS_EVENT_APPLIANCE,
opaque, cb);
@@ -318,6 +322,7 @@ void
guestfs_set_subprocess_quit_callback (guestfs_h *g,
guestfs_subprocess_quit_cb cb, void *opaque)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
replace_old_style_event_callback (g, subprocess_quit_callback_wrapper,
GUESTFS_EVENT_SUBPROCESS_QUIT,
opaque, cb);
@@ -340,6 +345,7 @@ void
guestfs_set_launch_done_callback (guestfs_h *g,
guestfs_launch_done_cb cb, void *opaque)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
replace_old_style_event_callback (g, launch_done_callback_wrapper,
GUESTFS_EVENT_LAUNCH_DONE,
opaque, cb);
@@ -362,6 +368,7 @@ void
guestfs_set_close_callback (guestfs_h *g,
guestfs_close_cb cb, void *opaque)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
replace_old_style_event_callback (g, close_callback_wrapper,
GUESTFS_EVENT_CLOSE,
opaque, cb);
@@ -385,6 +392,7 @@ void
guestfs_set_progress_callback (guestfs_h *g,
guestfs_progress_cb cb, void *opaque)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
replace_old_style_event_callback (g, progress_callback_wrapper,
GUESTFS_EVENT_PROGRESS,
opaque, cb);
diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index c4a14e962..37878b88d 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -78,6 +78,14 @@
#define TRACE4(name, arg1, arg2, arg3, arg4)
#endif
+/* Acquire and release the per-handle lock. Note the release happens
+ * in an __attribute__((cleanup)) handler, making it simple to write
+ * bug-free code.
+ */
+#define ACQUIRE_LOCK_FOR_CURRENT_SCOPE(g) \
+ CLEANUP_GL_RECURSIVE_LOCK_UNLOCK gl_recursive_lock_t *_lock = &(g)->lock; \
+ gl_recursive_lock_lock (*_lock)
+
/* Default and minimum appliance memory size. */
/* Needs to be larger on ppc64 because of the larger page size (64K).
diff --git a/lib/handle.c b/lib/handle.c
index 09c29ed84..183f247fb 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -322,6 +322,7 @@ guestfs_close (guestfs_h *g)
{
struct hv_param *hp, *hp_next;
guestfs_h **gg;
+ int r;
if (g->state == NO_HANDLE) {
/* Not safe to call ANY callbacks here, so ... */
@@ -402,7 +403,21 @@ guestfs_close (guestfs_h *g)
free (g->backend_data);
guestfs_int_free_string_list (g->backend_settings);
free (g->append);
- gl_recursive_lock_destroy (g->lock);
+ r = glthread_recursive_lock_destroy (&g->lock);
+ if (r != 0) {
+ /* If pthread_mutex_destroy returns 16 (EBUSY), this indicates
+ * that the lock is held somewhere. That means a programming
+ * error if the main program is using threads.
+ */
+ errno = r;
+ perror ("guestfs_close: g->lock");
+ /* While we're debugging locks in libguestfs I want this to fail
+ * noisily. Remove this later since there are valid times when
+ * this might fail such as if the program exits during a
+ * libguestfs operation.
+ */
+ abort ();
+ }
free (g);
}
diff --git a/lib/private-data.c b/lib/private-data.c
index f448894b4..65b902260 100644
--- a/lib/private-data.c
+++ b/lib/private-data.c
@@ -81,6 +81,7 @@ freer (void *x)
void
guestfs_set_private (guestfs_h *g, const char *key, void *data)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
struct pda_entry *new_entry, *old_entry, *entry;
if (g->pda == NULL) {
@@ -105,6 +106,8 @@ guestfs_set_private (guestfs_h *g, const char *key, void *data)
void *
guestfs_get_private (guestfs_h *g, const char *key)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
+
if (g->pda == NULL)
return NULL; /* no keys have been set */
@@ -120,6 +123,8 @@ guestfs_get_private (guestfs_h *g, const char *key)
void *
guestfs_first_private (guestfs_h *g, const char **key_rtn)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
+
if (g->pda == NULL)
return NULL;
@@ -139,6 +144,8 @@ guestfs_first_private (guestfs_h *g, const char **key_rtn)
void *
guestfs_next_private (guestfs_h *g, const char **key_rtn)
{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
+
if (g->pda == NULL)
return NULL;
--
2.13.0