Richard W.M. Jones
2015-Jun-06 13:20 UTC
[Libguestfs] [PATCH 0/5] Add support for thread-safe handle.
This patch isn't ready to go upstream. In fact, I think we might do a quick 1.30 release soon, and save this patch, and also the extensive changes proposed for the test suite[1], to after 1.30. Currently it is not safe to use the same handle from multiple threads, unless you implement your own mutexes. See: http://libguestfs.org/guestfs.3.html#multiple-handles-and-multiple-threads These patches add support for a thread-safe handle, so you can use the same handle concurrently from multiple threads without requiring the caller to perform any special locking. The implementation is fairly straightforward: a recursive lock is added to the handle (using the gnulib gl_recursive_lock so it should be fairly portable beyond POSIX threads). The recursive lock is required because libguestfs extensively calls itself. Also a recursive lock allows callbacks back to program code to work -- we don't want to release the lock before a callback since that would allow other threads to enter libguestfs using the same handle. I studied the libvirt implementation which doesn't use a recursive lock, but does use conditions to achieve the same thing more efficiently. It is considerably more complex. The difficult part of the implementation is error handling (see description of patch 3/5). This patch causes an API change, which is one reason why it cannot go upstream at the moment. The tests pass, but apart from running the tests standalone and under valgrind, and adding a (not very satisfactory) new test, I have not tested the patches any further. Comments welcome, Rich. [1] https://www.redhat.com/archives/libguestfs/2014-October/msg00208.html
Richard W.M. Jones
2015-Jun-06 13:20 UTC
[Libguestfs] [PATCH 1/5] threads: Add a lock (a recursive mutex) to the handle.
Add a g->lock field. This commit simply initializes and destroys the lock on handle creation/free, and does nothing else. --- src/guestfs-internal.h | 6 ++++++ src/handle.c | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index bbd7fb4..b68942f 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -33,6 +33,7 @@ #include <libvirt/libvirt.h> #endif +#include "glthread/lock.h" #include "hash.h" #include "guestfs-internal-frontend.h" @@ -365,6 +366,11 @@ struct guestfs_h struct guestfs_h *next; /* Linked list of open handles. */ enum state state; /* See the state machine diagram in guestfs(3)*/ + /* Lock acquired when entering any public guestfs_* function to + * protects the handle. + */ + gl_recursive_lock_define (, lock); + /**** Configuration of the handle. ****/ bool verbose; /* Debugging. */ bool trace; /* Trace calls. */ diff --git a/src/handle.c b/src/handle.c index 51b9572..a057475 100644 --- a/src/handle.c +++ b/src/handle.c @@ -84,6 +84,8 @@ guestfs_create_flags (unsigned flags, ...) g = calloc (1, sizeof (*g)); if (!g) return NULL; + gl_recursive_lock_init (g->lock); + g->state = CONFIG; g->conn = NULL; @@ -167,6 +169,7 @@ guestfs_create_flags (unsigned flags, ...) free (g->path); free (g->hv); free (g->append); + gl_recursive_lock_destroy (g->lock); free (g); return NULL; } @@ -389,6 +392,7 @@ 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); free (g); } -- 2.3.1
Richard W.M. Jones
2015-Jun-06 13:20 UTC
[Libguestfs] [PATCH 2/5] threads: Acquire and release the lock around each public guestfs_* API.
Since each ACQUIRE_LOCK/RELEASE_LOCK call must balance, this code is
difficult to debug. Enable DEBUG_LOCK to add some prints which can
help.
The only definitive list of public APIs is found indirectly in the
generator (in generator/c.ml : globals).
---
generator/c.ml | 18 ++++++++++++++
src/errors.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----
src/events.c | 49 +++++++++++++++++++++++++++++++------
src/guestfs-internal.h | 37 ++++++++++++++++++++++++++++
src/handle.c | 20 ++++++++++++++-
src/private-data.c | 47 +++++++++++++++++++++++++++++++----
6 files changed, 218 insertions(+), 19 deletions(-)
diff --git a/generator/c.ml b/generator/c.ml
index a2b9c94..93f1e5b 100644
--- a/generator/c.ml
+++ b/generator/c.ml
@@ -1271,6 +1271,7 @@ and generate_client_actions hash ()
"%s: RConstOptString function has invalid parameter '%s'"
c_name n
| (`ErrorIsMinusOne |`ErrorIsNULL) as e -> e in
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr_newline := true
@@ -1297,6 +1298,7 @@ and generate_client_actions hash () match
errcode_of_ret ret with
| `CannotReturnError -> assert false
| (`ErrorIsMinusOne |`ErrorIsNULL) as e -> e in
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr_newline := true
@@ -1311,6 +1313,7 @@ and generate_client_actions hash () match
errcode_of_ret ret with
| `CannotReturnError -> assert false
| (`ErrorIsMinusOne |`ErrorIsNULL) as e -> e in
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr_newline := true
@@ -1335,6 +1338,7 @@ and generate_client_actions hash () match
errcode_of_ret ret with
| `CannotReturnError -> assert false
| (`ErrorIsMinusOne |`ErrorIsNULL) as e -> e in
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr "\n";
@@ -1355,6 +1359,7 @@ and generate_client_actions hash () match
errcode_of_ret ret with
| `CannotReturnError -> assert false
| (`ErrorIsMinusOne |`ErrorIsNULL) as e -> e in
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr_newline := true
@@ -1589,10 +1594,12 @@ and generate_client_actions hash () pr "
struct guestfs_%s_list *r;\n" typ
);
pr "\n";
+ pr " ACQUIRE_LOCK (g);\n";
if config_only then (
pr " if (g->state != CONFIG) {\n";
pr " error (g, \"%%s: this function can only be called in
the config state\",\n";
pr " \"%s\");\n" c_name;
+ pr " RELEASE_LOCK (g);\n";
pr " return -1;\n";
pr " }\n";
);
@@ -1616,6 +1623,7 @@ and generate_client_actions hash () trace_return
name style "r";
);
pr "\n";
+ pr " RELEASE_LOCK (g);\n";
pr " return r;\n";
pr "}\n";
pr "\n"
@@ -1699,6 +1707,7 @@ and generate_client_actions hash () pr "
const uint64_t progress_hint = 0;\n";
pr "\n";
+ pr " ACQUIRE_LOCK (g);\n";
enter_event name;
check_null_strings c_name style;
reject_unknown_optargs c_name style;
@@ -1721,6 +1730,7 @@ and generate_client_actions hash () (* This is a
daemon_function so check the appliance is up. *)
pr " if (guestfs_int_check_appliance_up (g, \"%s\") == -1)
{\n" name;
trace_return_error ~indent:4 name style errcode;
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr "\n";
@@ -1754,6 +1764,7 @@ and generate_client_actions hash ()
trace_return_error ~indent:4 name style errcode;
pr " error (g, \"%%s: size of input buffer too
large\", \"%s\");\n"
name;
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr " args.%s.%s_val = (char *) %s;\n" n n n;
@@ -1798,6 +1809,7 @@ and generate_client_actions hash () );
pr " if (serial == -1) {\n";
trace_return_error ~indent:4 name style errcode;
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr "\n";
@@ -1812,6 +1824,7 @@ and generate_client_actions hash ()
trace_return_error ~indent:4 name style errcode;
pr " /* daemon will send an error reply which we discard
*/\n";
pr " guestfs_int_recv_discard (g, \"%s\");\n"
name;
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr " if (r == -2) /* daemon cancelled */\n";
@@ -1836,6 +1849,7 @@ and generate_client_actions hash ()
pr " if (r == -1) {\n";
trace_return_error ~indent:4 name style errcode;
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr "\n";
@@ -1843,6 +1857,7 @@ and generate_client_actions hash () pr " if
(guestfs_int_check_reply_header (g, &hdr, GUESTFS_PROC_%s, serial) == -1)
{\n"
(String.uppercase name);
trace_return_error ~indent:4 name style errcode;
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr "\n";
@@ -1862,6 +1877,7 @@ and generate_client_actions hash () pr "
err.error_message);\n";
pr " free (err.error_message);\n";
pr " free (err.errno_string);\n";
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr "\n";
@@ -1872,6 +1888,7 @@ and generate_client_actions hash () | FileOut n
->
pr " if (guestfs_int_recv_file (g, %s) == -1) {\n" n;
trace_return_error ~indent:4 name style errcode;
+ pr " RELEASE_LOCK (g);\n";
pr " return %s;\n" (string_of_errcode errcode);
pr " }\n";
pr "\n";
@@ -1918,6 +1935,7 @@ and generate_client_actions hash () pr "
}\n";
);
trace_return name style "ret_v";
+ pr " RELEASE_LOCK (g);\n";
pr " return ret_v;\n";
pr "}\n\n"
in
diff --git a/src/errors.c b/src/errors.c
index 2d3ae84..d9959b2 100644
--- a/src/errors.c
+++ b/src/errors.c
@@ -29,16 +29,38 @@
#include "guestfs.h"
#include "guestfs-internal.h"
+static const char *
+unlocked_last_error (guestfs_h *g)
+{
+ return g->last_error;
+}
+
const char *
guestfs_last_error (guestfs_h *g)
{
- return g->last_error;
+ const char *r;
+
+ ACQUIRE_LOCK (g);
+ r = unlocked_last_error (g);
+ RELEASE_LOCK (g);
+ return r;
+}
+
+static int
+unlocked_last_errno (guestfs_h *g)
+{
+ return g->last_errnum;
}
int
guestfs_last_errno (guestfs_h *g)
{
- return g->last_errnum;
+ int r;
+
+ ACQUIRE_LOCK (g);
+ r = unlocked_last_errno (g);
+ RELEASE_LOCK (g);
+ return r;
}
static void
@@ -164,36 +186,64 @@ 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 (g);
g->abort_cb = cb;
+ RELEASE_LOCK (g);
+}
+
+static guestfs_abort_cb
+unlocked_get_out_of_memory_handler (guestfs_h *g)
+{
+ return g->abort_cb;
}
guestfs_abort_cb
guestfs_get_out_of_memory_handler (guestfs_h *g)
{
- return g->abort_cb;
+ guestfs_abort_cb r;
+
+ ACQUIRE_LOCK (g);
+ r = unlocked_get_out_of_memory_handler (g);
+ RELEASE_LOCK (g);
+ return r;
}
void
guestfs_set_error_handler (guestfs_h *g,
guestfs_error_handler_cb cb, void *data)
{
+ ACQUIRE_LOCK (g);
g->error_cb = cb;
g->error_cb_data = data;
+ RELEASE_LOCK (g);
}
-guestfs_error_handler_cb
-guestfs_get_error_handler (guestfs_h *g, void **data_rtn)
+static guestfs_error_handler_cb
+unlocked_get_error_handler (guestfs_h *g, void **data_rtn)
{
if (data_rtn) *data_rtn = g->error_cb_data;
return g->error_cb;
}
+guestfs_error_handler_cb
+guestfs_get_error_handler (guestfs_h *g, void **data_rtn)
+{
+ guestfs_error_handler_cb r;
+
+ ACQUIRE_LOCK (g);
+ r = unlocked_get_error_handler (g, data_rtn);
+ RELEASE_LOCK (g);
+ return r;
+}
+
void
guestfs_push_error_handler (guestfs_h *g,
guestfs_error_handler_cb cb, void *data)
{
struct error_cb_stack *old_stack;
+ ACQUIRE_LOCK (g);
+
old_stack = g->error_cb_stack;
g->error_cb_stack = safe_malloc (g, sizeof (struct error_cb_stack));
g->error_cb_stack->next = old_stack;
@@ -201,6 +251,8 @@ guestfs_push_error_handler (guestfs_h *g,
g->error_cb_stack->error_cb_data = g->error_cb_data;
guestfs_set_error_handler (g, cb, data);
+
+ RELEASE_LOCK (g);
}
void
@@ -208,6 +260,8 @@ guestfs_pop_error_handler (guestfs_h *g)
{
struct error_cb_stack *next_stack;
+ ACQUIRE_LOCK (g);
+
if (g->error_cb_stack) {
next_stack = g->error_cb_stack->next;
guestfs_set_error_handler (g, g->error_cb_stack->error_cb,
@@ -217,6 +271,8 @@ guestfs_pop_error_handler (guestfs_h *g)
}
else
guestfs_int_init_error_handler (g);
+
+ RELEASE_LOCK (g);
}
static void default_error_cb (guestfs_h *g, void *data, const char *msg);
diff --git a/src/events.c b/src/events.c
index 51b9948..72a58aa 100644
--- a/src/events.c
+++ b/src/events.c
@@ -32,12 +32,12 @@
#include "guestfs.h"
#include "guestfs-internal.h"
-int
-guestfs_set_event_callback (guestfs_h *g,
- guestfs_event_callback cb,
- uint64_t event_bitmask,
- int flags,
- void *opaque)
+static int
+unlocked_set_event_callback (guestfs_h *g,
+ guestfs_event_callback cb,
+ uint64_t event_bitmask,
+ int flags,
+ void *opaque)
{
int event_handle;
@@ -70,8 +70,23 @@ guestfs_set_event_callback (guestfs_h *g,
return event_handle;
}
-void
-guestfs_delete_event_callback (guestfs_h *g, int event_handle)
+int
+guestfs_set_event_callback (guestfs_h *g,
+ guestfs_event_callback cb,
+ uint64_t event_bitmask,
+ int flags,
+ void *opaque)
+{
+ int r;
+
+ ACQUIRE_LOCK (g);
+ r = unlocked_set_event_callback (g, cb, event_bitmask, flags, opaque);
+ RELEASE_LOCK (g);
+ return r;
+}
+
+static void
+unlocked_delete_event_callback (guestfs_h *g, int event_handle)
{
if (event_handle < 0 || event_handle >= (int) g->nr_events)
return;
@@ -91,6 +106,14 @@ guestfs_delete_event_callback (guestfs_h *g, int
event_handle)
g->nr_events--;
}
+void
+guestfs_delete_event_callback (guestfs_h *g, int event_handle)
+{
+ ACQUIRE_LOCK (g);
+ unlocked_delete_event_callback (g, event_handle);
+ RELEASE_LOCK (g);
+}
+
/* Functions to generate an event with various payloads. */
void
@@ -295,9 +318,11 @@ void
guestfs_set_log_message_callback (guestfs_h *g,
guestfs_log_message_cb cb, void *opaque)
{
+ ACQUIRE_LOCK (g);
replace_old_style_event_callback (g, log_message_callback_wrapper,
GUESTFS_EVENT_APPLIANCE,
opaque, cb);
+ RELEASE_LOCK (g);
}
static void
@@ -317,9 +342,11 @@ void
guestfs_set_subprocess_quit_callback (guestfs_h *g,
guestfs_subprocess_quit_cb cb, void
*opaque)
{
+ ACQUIRE_LOCK (g);
replace_old_style_event_callback (g, subprocess_quit_callback_wrapper,
GUESTFS_EVENT_SUBPROCESS_QUIT,
opaque, cb);
+ RELEASE_LOCK (g);
}
static void
@@ -339,9 +366,11 @@ void
guestfs_set_launch_done_callback (guestfs_h *g,
guestfs_launch_done_cb cb, void *opaque)
{
+ ACQUIRE_LOCK (g);
replace_old_style_event_callback (g, launch_done_callback_wrapper,
GUESTFS_EVENT_LAUNCH_DONE,
opaque, cb);
+ RELEASE_LOCK (g);
}
static void
@@ -361,9 +390,11 @@ void
guestfs_set_close_callback (guestfs_h *g,
guestfs_close_cb cb, void *opaque)
{
+ ACQUIRE_LOCK (g);
replace_old_style_event_callback (g, close_callback_wrapper,
GUESTFS_EVENT_CLOSE,
opaque, cb);
+ RELEASE_LOCK (g);
}
static void
@@ -384,7 +415,9 @@ void
guestfs_set_progress_callback (guestfs_h *g,
guestfs_progress_cb cb, void *opaque)
{
+ ACQUIRE_LOCK (g);
replace_old_style_event_callback (g, progress_callback_wrapper,
GUESTFS_EVENT_PROGRESS,
opaque, cb);
+ RELEASE_LOCK (g);
}
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index b68942f..6601096 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -58,6 +58,40 @@
#define TRACE4(name, arg1, arg2, arg3, arg4)
#endif
+/* Debug per-handle locks. */
+//#define DEBUG_LOCK 1
+
+/* Acquire and release the per-handle lock. */
+#ifdef DEBUG_LOCK
+#include <errno.h>
+#define ACQUIRE_LOCK(g) do { \
+ int _lock_err = glthread_recursive_lock_lock (&(g)->lock);
\
+ if (_lock_err == 0) { \
+ (g)->lock_count++; \
+ printf ("%s: %d: ACQUIRE_LOCK: lock count = %d\n",
\
+ __FILE__, __LINE__, (g)->lock_count); \
+ } else { \
+ errno = _lock_err; \
+ fprintf (stderr, "%s: %d: ACQUIRE_LOCK", __FILE__, __LINE__);
\
+ abort (); \
+ } \
+ } while (0)
+#define RELEASE_LOCK(g) do { \
+ (g)->lock_count--; \
+ printf ("%s: %d: RELEASE_LOCK: lock count = %d\n",
\
+ __FILE__, __LINE__, (g)->lock_count); \
+ int _lock_err = glthread_recursive_lock_unlock (&(g)->lock);
\
+ if (_lock_err != 0) { \
+ errno = _lock_err; \
+ fprintf (stderr, "%s: %d: RELEASE_LOCK", __FILE__, __LINE__);
\
+ abort (); \
+ } \
+ } while (0)
+#else /* !DEBUG_LOCK */
+#define ACQUIRE_LOCK(g) gl_recursive_lock_lock ((g)->lock)
+#define RELEASE_LOCK(g) gl_recursive_lock_unlock ((g)->lock)
+#endif
+
/* Default and minimum appliance memory size. */
/* Needs to be larger on ppc64 because of the larger page size (64K).
@@ -370,6 +404,9 @@ struct guestfs_h
* protects the handle.
*/
gl_recursive_lock_define (, lock);
+#ifdef DEBUG_LOCK
+ int lock_count;
+#endif
/**** Configuration of the handle. ****/
bool verbose; /* Debugging. */
diff --git a/src/handle.c b/src/handle.c
index a057475..dfb8817 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -316,6 +316,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 ... */
@@ -392,7 +393,24 @@ 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 either a
+ * programming error if the main program is using threads, or that
+ * there is an unbalanced ACQUIRE_LOCK/RELEASE_LOCK pair somewhere
+ * in libguestfs. Enable DEBUG_LOCK in guestfs-internal.h to
+ * diagnose these problems.
+ */
+ 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/src/private-data.c b/src/private-data.c
index 725b74b..d54033b 100644
--- a/src/private-data.c
+++ b/src/private-data.c
@@ -68,6 +68,8 @@ guestfs_set_private (guestfs_h *g, const char *key, void
*data)
{
struct pda_entry *new_entry, *old_entry, *entry;
+ ACQUIRE_LOCK (g);
+
if (g->pda == NULL) {
g->pda = hash_initialize (16, NULL, hasher, comparator, freer);
if (g->pda == NULL)
@@ -85,10 +87,12 @@ guestfs_set_private (guestfs_h *g, const char *key, void
*data)
if (entry == NULL)
g->abort_cb ();
assert (entry == new_entry);
+
+ RELEASE_LOCK (g);
}
-void *
-guestfs_get_private (guestfs_h *g, const char *key)
+static void *
+unlocked_get_private (guestfs_h *g, const char *key)
{
if (g->pda == NULL)
return NULL; /* no keys have been set */
@@ -101,9 +105,20 @@ guestfs_get_private (guestfs_h *g, const char *key)
return NULL;
}
+void *
+guestfs_get_private (guestfs_h *g, const char *key)
+{
+ void *r;
+
+ ACQUIRE_LOCK (g);
+ r = unlocked_get_private (g, key);
+ RELEASE_LOCK (g);
+ return r;
+}
+
/* Iterator. */
-void *
-guestfs_first_private (guestfs_h *g, const char **key_rtn)
+static void *
+unlocked_first_private (guestfs_h *g, const char **key_rtn)
{
if (g->pda == NULL)
return NULL;
@@ -122,7 +137,18 @@ guestfs_first_private (guestfs_h *g, const char **key_rtn)
}
void *
-guestfs_next_private (guestfs_h *g, const char **key_rtn)
+guestfs_first_private (guestfs_h *g, const char **key_rtn)
+{
+ void *r;
+
+ ACQUIRE_LOCK (g);
+ r = unlocked_first_private (g, key_rtn);
+ RELEASE_LOCK (g);
+ return r;
+}
+
+static void *
+unlocked_next_private (guestfs_h *g, const char **key_rtn)
{
if (g->pda == NULL)
return NULL;
@@ -141,3 +167,14 @@ guestfs_next_private (guestfs_h *g, const char **key_rtn)
*key_rtn = g->pda_next->key;
return g->pda_next->data;
}
+
+void *
+guestfs_next_private (guestfs_h *g, const char **key_rtn)
+{
+ void *r;
+
+ ACQUIRE_LOCK (g);
+ r = unlocked_next_private (g, key_rtn);
+ RELEASE_LOCK (g);
+ return r;
+}
--
2.3.1
Richard W.M. Jones
2015-Jun-06 13:20 UTC
[Libguestfs] [PATCH 3/5] threads: Use thread-local storage for errors.
We permit the following constructs in libguestfs code:
if (guestfs_some_call (g) == -1) {
fprintf (stderr, "failed: error is %s\n", guestfs_last_error (g));
}
and:
guestfs_push_error_handler (g, NULL, NULL);
guestfs_some_call (g);
guestfs_pop_error_handler (g);
Neither of these would be safe if we allowed the handle to be used
from threads concurrently, since the error string or error handler
could be changed by another thread.
Solve this in approximately the same way that libvirt does: by making
the error, current error handler, and stack of error handlers use
thread-local storage (TLS).
The implementation is not entirely straightforward, mainly because
POSIX doesn't give us useful destructor behaviour, so effectively we
end up creating our own destructor using a linked list.
Note that you have to set the error handler in each thread separately,
which is an API change (eg: if you set the error handler in one
thread, then pass the handle 'g' to another thread, the error handler
in the second thread appears to have reset itself back to the default
error handler). I haven't yet worked out a better way to solve this.
---
bootstrap | 1 +
m4/.gitignore | 1 +
src/errors.c | 196 ++++++++++++++++++++++++++++++++++++++++---------
src/guestfs-internal.h | 23 +++---
src/handle.c | 11 +--
5 files changed, 180 insertions(+), 52 deletions(-)
diff --git a/bootstrap b/bootstrap
index 5df6f0f..7733f8f 100755
--- a/bootstrap
+++ b/bootstrap
@@ -91,6 +91,7 @@ strndup
symlinkat
sys_select
sys_wait
+tls
vasprintf
vc-list-files
warnings
diff --git a/m4/.gitignore b/m4/.gitignore
index eff909a..9a26217 100644
--- a/m4/.gitignore
+++ b/m4/.gitignore
@@ -241,6 +241,7 @@
/thread.m4
/time_h.m4
/timespec.m4
+/tls.m4
/ttyname_r.m4
/uintmax_t.m4
/ulonglong.m4
diff --git a/src/errors.c b/src/errors.c
index d9959b2..d5d0308 100644
--- a/src/errors.c
+++ b/src/errors.c
@@ -29,10 +29,132 @@
#include "guestfs.h"
#include "guestfs-internal.h"
+/* How errors and error handlers works in the handle:
+ *
+ * The handle has a g->error_data field which is a thread-local
+ * storage (TLS) key.
+ *
+ * We use TLS because we want to support the common idioms of:
+ * if (guestfs_foo (g) == -1)
+ * printf ("%s\n", guestfs_last_error (g));
+ * and:
+ * guestfs_push_error_handler (g, ...);
+ * guestfs_foo (g);
+ * guestfs_pop_error_handler (g);
+ * neither of which would ordinarily be safe when using the same
+ * handle from multiple threads.
+ *
+ * In each thread, the TLS data is either NULL or contains a pointer
+ * to a 'struct error_data'.
+ *
+ * When it is NULL, it means the stack is empty (in that thread) and
+ * the default handler (default_error_cb) is installed.
+ *
+ * As soon as the current thread calls guestfs_set_error_handler,
+ * guestfs_push_error_handler, or an error is set in the handle (calls
+ * like guestfs_int_perrorf and so on), the key is created and
+ * initialized with a pointer to a real 'struct error_data'.
+ *
+ * All the 'struct error_data' structures associated with one handle
+ * are linked together in a linked list, so that we are able to free
+ * them when the handle is closed. (The pthread_key* API doesn't give
+ * us any other way to do this, in particular pthread_key_destroy
+ * doesn't call the destructor associated with the key).
+ */
+
+static void default_error_cb (guestfs_h *g, void *data, const char *msg);
+
+/* Stack of old error handlers. */
+struct error_cb_stack {
+ struct error_cb_stack *next;
+ guestfs_error_handler_cb error_cb;
+ void * error_cb_data;
+};
+
+/* Error data, stored in thread-local storage in g->error_data key. */
+struct error_data {
+ /* Linked list of error_data structs allocated for this handle. */
+ struct error_data *next;
+
+ char *last_error; /* Last error on handle. */
+ int last_errnum; /* errno, or 0 if there was no errno */
+
+ /* Error handler and stack of old error handlers. */
+ guestfs_error_handler_cb error_cb;
+ void * error_cb_data;
+ struct error_cb_stack *error_cb_stack;
+};
+
+static void
+free_error_data (struct error_data *error_data)
+{
+ struct error_cb_stack *p, *next_p;
+
+ free (error_data->last_error);
+ for (p = error_data->error_cb_stack; p != NULL; p = next_p) {
+ next_p = p->next;
+ free (p);
+ }
+ free (error_data);
+}
+
+/* Free all the error_data structs created for a particular handle. */
+void
+guestfs_int_free_error_data_list (guestfs_h *g)
+{
+ struct error_data *p, *next_p;
+
+ gl_lock_lock (g->error_data_list_lock);
+
+ for (p = g->error_data_list; p != NULL; p = next_p) {
+ next_p = p->next;
+ free_error_data (p);
+ }
+
+ g->error_data_list = NULL;
+
+ gl_lock_unlock (g->error_data_list_lock);
+}
+
+/* Get thread-specific error_data struct. Create it if necessary. */
+static struct error_data *
+get_error_data (guestfs_h *g)
+{
+ struct error_data *ret;
+
+ ret = gl_tls_get (g->error_data);
+
+ /* Not allocated yet for this thread, so allocate one. */
+ if (ret == NULL) {
+ ret = safe_malloc (g, sizeof *ret);
+ ret->last_error = NULL;
+ ret->last_errnum = 0;
+ ret->error_cb = default_error_cb;
+ ret->error_cb_data = NULL;
+ ret->error_cb_stack = NULL;
+
+ /* Add it to the linked list of struct error_data that are
+ * associated with this handle, so we can free them when the
+ * handle is closed.
+ */
+ gl_lock_lock (g->error_data_list_lock);
+ ret->next = g->error_data_list;
+ g->error_data_list = ret;
+ gl_lock_unlock (g->error_data_list_lock);
+
+ /* Set the TLS to point to the struct. This is safe because we
+ * should have acquired the handle lock.
+ */
+ gl_tls_set (g->error_data, ret);
+ }
+
+ return ret;
+}
+
static const char *
unlocked_last_error (guestfs_h *g)
{
- return g->last_error;
+ return get_error_data (g)->last_error;
}
const char *
@@ -49,7 +171,7 @@ guestfs_last_error (guestfs_h *g)
static int
unlocked_last_errno (guestfs_h *g)
{
- return g->last_errnum;
+ return get_error_data (g)->last_errnum;
}
int
@@ -66,9 +188,11 @@ guestfs_last_errno (guestfs_h *g)
static void
set_last_error (guestfs_h *g, int errnum, const char *msg)
{
- free (g->last_error);
- g->last_error = strdup (msg);
- g->last_errnum = errnum;
+ struct error_data *error_data = get_error_data (g);
+
+ free (error_data->last_error);
+ error_data->last_error = strdup (msg);
+ error_data->last_errnum = errnum;
}
/* Warning are printed unconditionally. We try to make these rare.
@@ -141,6 +265,7 @@ guestfs_int_error_errno (guestfs_h *g, int errnum, const
char *fs, ...)
va_list args;
CLEANUP_FREE char *msg = NULL;
int err;
+ struct error_data *error_data = get_error_data (g);
va_start (args, fs);
err = vasprintf (&msg, fs, args);
@@ -152,7 +277,8 @@ guestfs_int_error_errno (guestfs_h *g, int errnum, const
char *fs, ...)
* message and errno through the handle if it wishes.
*/
set_last_error (g, errnum, msg);
- if (g->error_cb) g->error_cb (g, g->error_cb_data, msg);
+ if (error_data->error_cb)
+ error_data->error_cb (g, error_data->error_cb_data, msg);
}
void
@@ -163,6 +289,7 @@ guestfs_int_perrorf (guestfs_h *g, const char *fs, ...)
int errnum = errno;
int err;
char buf[256];
+ struct error_data *error_data = get_error_data (g);
va_start (args, fs);
err = vasprintf (&msg, fs, args);
@@ -180,7 +307,8 @@ guestfs_int_perrorf (guestfs_h *g, const char *fs, ...)
* message and errno through the handle if it wishes.
*/
set_last_error (g, errnum, msg);
- if (g->error_cb) g->error_cb (g, g->error_cb_data, msg);
+ if (error_data->error_cb)
+ error_data->error_cb (g, error_data->error_cb_data, msg);
}
void
@@ -212,17 +340,22 @@ void
guestfs_set_error_handler (guestfs_h *g,
guestfs_error_handler_cb cb, void *data)
{
+ struct error_data *error_data;
+
ACQUIRE_LOCK (g);
- g->error_cb = cb;
- g->error_cb_data = data;
+ error_data = get_error_data (g);
+ error_data->error_cb = cb;
+ error_data->error_cb_data = data;
RELEASE_LOCK (g);
}
static guestfs_error_handler_cb
unlocked_get_error_handler (guestfs_h *g, void **data_rtn)
{
- if (data_rtn) *data_rtn = g->error_cb_data;
- return g->error_cb;
+ struct error_data *error_data = get_error_data (g);
+
+ if (data_rtn) *data_rtn = error_data->error_cb_data;
+ return error_data->error_cb;
}
guestfs_error_handler_cb
@@ -240,15 +373,17 @@ void
guestfs_push_error_handler (guestfs_h *g,
guestfs_error_handler_cb cb, void *data)
{
+ struct error_data *error_data;
struct error_cb_stack *old_stack;
ACQUIRE_LOCK (g);
- old_stack = g->error_cb_stack;
- g->error_cb_stack = safe_malloc (g, sizeof (struct error_cb_stack));
- g->error_cb_stack->next = old_stack;
- g->error_cb_stack->error_cb = g->error_cb;
- g->error_cb_stack->error_cb_data = g->error_cb_data;
+ error_data = get_error_data (g);
+ old_stack = error_data->error_cb_stack;
+ error_data->error_cb_stack = safe_malloc (g, sizeof (struct
error_cb_stack));
+ error_data->error_cb_stack->next = old_stack;
+ error_data->error_cb_stack->error_cb = error_data->error_cb;
+ error_data->error_cb_stack->error_cb_data =
error_data->error_cb_data;
guestfs_set_error_handler (g, cb, data);
@@ -258,32 +393,27 @@ guestfs_push_error_handler (guestfs_h *g,
void
guestfs_pop_error_handler (guestfs_h *g)
{
+ struct error_data *error_data;
struct error_cb_stack *next_stack;
ACQUIRE_LOCK (g);
- if (g->error_cb_stack) {
- next_stack = g->error_cb_stack->next;
- guestfs_set_error_handler (g, g->error_cb_stack->error_cb,
- g->error_cb_stack->error_cb_data);
- free (g->error_cb_stack);
- g->error_cb_stack = next_stack;
+ error_data = get_error_data (g);
+ if (error_data->error_cb_stack) {
+ next_stack = error_data->error_cb_stack->next;
+ guestfs_set_error_handler (g, error_data->error_cb_stack->error_cb,
+
error_data->error_cb_stack->error_cb_data);
+ free (error_data->error_cb_stack);
+ error_data->error_cb_stack = next_stack;
+ }
+ else {
+ error_data->error_cb = default_error_cb;
+ error_data->error_cb_data = NULL;
}
- else
- guestfs_int_init_error_handler (g);
RELEASE_LOCK (g);
}
-static void default_error_cb (guestfs_h *g, void *data, const char *msg);
-
-void
-guestfs_int_init_error_handler (guestfs_h *g)
-{
- g->error_cb = default_error_cb;
- g->error_cb_data = NULL;
-}
-
static void
default_error_cb (guestfs_h *g, void *data, const char *msg)
{
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 6601096..1416c02 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -34,6 +34,7 @@
#endif
#include "glthread/lock.h"
+#include "glthread/tls.h"
#include "hash.h"
#include "guestfs-internal-frontend.h"
@@ -387,13 +388,6 @@ struct connection_ops {
int (*can_read_data) (guestfs_h *g, struct connection *);
};
-/* Stack of old error handlers. */
-struct error_cb_stack {
- struct error_cb_stack *next;
- guestfs_error_handler_cb error_cb;
- void * error_cb_data;
-};
-
/* The libguestfs handle. */
struct guestfs_h
{
@@ -462,9 +456,6 @@ struct guestfs_h
char **backend_settings; /* Backend settings (can be NULL). */
/**** Runtime information. ****/
- char *last_error; /* Last error on handle. */
- int last_errnum; /* errno, or 0 if there was no errno */
-
/* Temporary and cache directories. */
/* The actual temporary directory - this is not created with the
* handle, you have to call guestfs_int_lazy_make_tmpdir.
@@ -476,9 +467,13 @@ struct guestfs_h
char *int_cachedir; /* $LIBGUESTFS_CACHEDIR or guestfs_set_cachedir or NULL
*/
/* Error handler, plus stack of old error handlers. */
- guestfs_error_handler_cb error_cb;
- void * error_cb_data;
- struct error_cb_stack *error_cb_stack;
+ gl_tls_key_t error_data;
+
+ /* Linked list of error_data structures allocated for this handle,
+ * plus a mutex to protect the linked list.
+ */
+ gl_lock_define (, error_data_list_lock);
+ struct error_data *error_data_list;
/* Out of memory error handler. */
guestfs_abort_cb abort_cb;
@@ -657,7 +652,7 @@ struct guestfs_progress;
extern int guestfs_int_get_backend_setting_bool (guestfs_h *g, const char
*name);
/* errors.c */
-extern void guestfs_int_init_error_handler (guestfs_h *g);
+extern void guestfs_int_free_error_data_list (guestfs_h *g);
extern void guestfs_int_error_errno (guestfs_h *g, int errnum, const char *fs,
...)
__attribute__((format (printf,3,4)));
diff --git a/src/handle.c b/src/handle.c
index dfb8817..ba7928d 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -32,6 +32,7 @@
#include <libxml/xmlversion.h>
#include "glthread/lock.h"
+#include "glthread/tls.h"
#include "ignore-value.h"
#include "guestfs.h"
@@ -90,7 +91,7 @@ guestfs_create_flags (unsigned flags, ...)
g->conn = NULL;
- guestfs_int_init_error_handler (g);
+ gl_tls_key_init (g->error_data, NULL);
g->abort_cb = abort;
g->recovery_proc = 1;
@@ -169,6 +170,8 @@ guestfs_create_flags (unsigned flags, ...)
free (g->path);
free (g->hv);
free (g->append);
+ guestfs_int_free_error_data_list (g);
+ gl_tls_key_destroy (g->error_data);
gl_recursive_lock_destroy (g->lock);
free (g);
return NULL;
@@ -376,16 +379,12 @@ guestfs_close (guestfs_h *g)
free (hp);
}
- while (g->error_cb_stack)
- guestfs_pop_error_handler (g);
-
if (g->pda)
hash_free (g->pda);
free (g->tmpdir);
free (g->env_tmpdir);
free (g->int_tmpdir);
free (g->int_cachedir);
- free (g->last_error);
free (g->program);
free (g->path);
free (g->hv);
@@ -393,6 +392,8 @@ guestfs_close (guestfs_h *g)
free (g->backend_data);
guestfs_int_free_string_list (g->backend_settings);
free (g->append);
+ guestfs_int_free_error_data_list (g);
+ gl_tls_key_destroy (g->error_data);
r = glthread_recursive_lock_destroy (&g->lock);
if (r != 0) {
/* If pthread_mutex_destroy returns 16 (EBUSY), this indicates
--
2.3.1
Richard W.M. Jones
2015-Jun-06 13:20 UTC
[Libguestfs] [PATCH 4/5] threads: Update documentation in guestfs(3) to describe the new behaviour.
--- src/guestfs.pod | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/src/guestfs.pod b/src/guestfs.pod index e2ee1b5..d7abd79 100644 --- a/src/guestfs.pod +++ b/src/guestfs.pod @@ -1319,14 +1319,31 @@ encrypted devices. All high-level libguestfs actions are synchronous. If you want to use libguestfs asynchronously then you must create a thread. -Only use the handle from a single thread. Either use the handle -exclusively from one thread, or provide your own mutex so that two -threads cannot issue calls on the same handle at the same time. Even -apparently innocent functions like L</guestfs_get_trace> are I<not> -safe to be called from multiple threads without a mutex. - -See the graphical program guestfs-browser for one possible -architecture for multithreaded programs using libvirt and libguestfs. +=head3 Threads in libguestfs E<ge> 1.30 + +In libguestfs E<ge> 1.30, each handle (C<guestfs_h>) contains a lock +which is acquired automatically when you call a libguestfs function. +The practical effect of this is you can call libguestfs functions with +the same handle from multiple threads without needing to do any +locking. + +Also in libguestfs E<ge> 1.30, the last error on the handle +(L</guestfs_last_error>, L</guestfs_last_errno>) is stored in +thread-local storage, so it is safe to write code like: + + if (guestfs_add_drive_ro (g, drive) == -1) + fprintf (stderr, "error was: %s\n", guestfs_last_error (g)); + +even when other threads may be concurrently using the same handle C<g>. + +=head3 Threads in libguestfs E<lt> 1.30 + +In libguestfs E<lt> 1.30, you must use the handle only from a single +thread. Either use the handle exclusively from one thread, or provide +your own mutex so that two threads cannot issue calls on the same +handle at the same time. Even apparently innocent functions like +L</guestfs_get_trace> are I<not> safe to be called from multiple +threads without a mutex in libguestfs E<lt> 1.30. =head2 PATH -- 2.3.1
---
.gitignore | 1 +
tests/c-api/Makefile.am | 20 ++++++-
tests/c-api/test-threads.c | 133 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 152 insertions(+), 2 deletions(-)
create mode 100644 tests/c-api/test-threads.c
diff --git a/.gitignore b/.gitignore
index d1292d9..e028468 100644
--- a/.gitignore
+++ b/.gitignore
@@ -504,6 +504,7 @@ Makefile.in
/tests/c-api/test-pwd
/tests/c-api/tests
/tests/c-api/tests.c
+/tests/c-api/test-threads
/tests/c-api/test*.tmp
/tests/c-api/test-user-cancel
/tests/charsets/test-charset-fidelity
diff --git a/tests/c-api/Makefile.am b/tests/c-api/Makefile.am
index 7bfffe5..55e82b2 100644
--- a/tests/c-api/Makefile.am
+++ b/tests/c-api/Makefile.am
@@ -37,7 +37,8 @@ check_PROGRAMS = \
test-debug-to-file \
test-environment \
test-pwd \
- test-event-string
+ test-event-string \
+ test-threads
if HAVE_LIBDL
check_PROGRAMS += \
test-dlopen
@@ -55,7 +56,8 @@ TESTS = \
test-user-cancel \
test-debug-to-file \
test-environment \
- test-event-string
+ test-event-string \
+ test-threads
if HAVE_LIBDL
TESTS += \
test-dlopen
@@ -238,6 +240,20 @@ test_event_string_LDADD = \
$(LTLIBINTL) \
$(top_builddir)/gnulib/lib/libgnu.la
+test_threads_SOURCES = test-threads.c
+test_threads_CPPFLAGS = \
+ -I$(top_srcdir)/src -I$(top_builddir)/src \
+ -I$(top_srcdir)/gnulib/lib \
+ -I$(top_builddir)/gnulib/lib
+test_threads_CFLAGS = \
+ -pthread \
+ $(WARN_CFLAGS) $(WERROR_CFLAGS)
+test_threads_LDADD = \
+ $(top_builddir)/src/libguestfs.la \
+ $(LTLIBTHREAD) \
+ $(LTLIBINTL) \
+ $(top_builddir)/gnulib/lib/libgnu.la
+
if HAVE_LIBVIRT
test_add_libvirt_dom_SOURCES = test-add-libvirt-dom.c
test_add_libvirt_dom_CPPFLAGS = \
diff --git a/tests/c-api/test-threads.c b/tests/c-api/test-threads.c
new file mode 100644
index 0000000..218080f
--- /dev/null
+++ b/tests/c-api/test-threads.c
@@ -0,0 +1,133 @@
+/* libguestfs
+ * Copyright (C) 2015 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+/* Test that we can make API calls safely from multiple threads. */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+#include <assert.h>
+
+#include <pthread.h>
+
+#include "guestfs.h"
+#include "guestfs-internal-frontend.h"
+
+static guestfs_h *g;
+
+#define RUN_TIME 60 /* seconds */
+#define NR_CONCURRENT_THREADS 4
+
+static void *start_thread (void *nullv);
+
+int
+main (int argc, char *argv[])
+{
+ time_t start_t, t;
+ pthread_t threads[NR_CONCURRENT_THREADS];
+ void *ret;
+ int i, r;
+
+ /* Because we rely on error message content below, force LC_ALL=C. */
+ setenv ("LC_ALL", "C", 1);
+
+ g = guestfs_create ();
+ if (!g) {
+ perror ("guestfs_create");
+ exit (EXIT_FAILURE);
+ }
+
+ time (&start_t);
+
+ while (time (&t), t - start_t < RUN_TIME) {
+ for (i = 0; i < NR_CONCURRENT_THREADS; ++i) {
+ r = pthread_create (&threads[i], NULL, start_thread, NULL);
+ if (r != 0) {
+ fprintf (stderr, "pthread_create: %s\n", strerror (r));
+ exit (EXIT_FAILURE);
+ }
+ }
+
+ for (i = 0; i < NR_CONCURRENT_THREADS; ++i) {
+ r = pthread_join (threads[i], &ret);
+ if (r != 0) {
+ fprintf (stderr, "pthread_join: %s\n", strerror (r));
+ exit (EXIT_FAILURE);
+ }
+ if (ret != NULL) {
+ fprintf (stderr, "thread[%d] failed\n", i);
+ exit (EXIT_FAILURE);
+ }
+ }
+ }
+
+ guestfs_close (g);
+
+ exit (EXIT_SUCCESS);
+}
+
+static void *
+start_thread (void *nullv)
+{
+ char *p;
+ const char *err;
+ int iterations;
+
+ for (iterations = 0; iterations < 1000; ++iterations) {
+ guestfs_set_hv (g, "test");
+ p = guestfs_get_hv (g);
+ if (!p || STRNEQ (p, "test")) {
+ fprintf (stderr, "invalid return from guestfs_get_hv\n");
+ pthread_exit ((void *)-1);
+ }
+ free (p);
+
+ guestfs_push_error_handler (g, NULL, NULL);
+ guestfs_set_hv (g, "test");
+ p = guestfs_get_hv (g);
+ guestfs_pop_error_handler (g);
+ if (!p || STRNEQ (p, "test")) {
+ fprintf (stderr, "invalid return from guestfs_get_hv\n");
+ pthread_exit ((void *)-1);
+ }
+ free (p);
+
+ guestfs_push_error_handler (g, NULL, NULL);
+ guestfs_set_program (g, NULL); /* deliberately cause an error */
+ guestfs_pop_error_handler (g);
+ err = guestfs_last_error (g);
+ if (!err || !STRPREFIX (err, "set_program: program: ")) {
+ fprintf (stderr, "invalid error message: %s\n", err ? err :
"NULL");
+ pthread_exit ((void *)-1);
+ }
+
+ guestfs_push_error_handler (g, NULL, NULL);
+ guestfs_set_memsize (g, 1); /* deliberately cause an error */
+ guestfs_pop_error_handler (g);
+ err = guestfs_last_error (g);
+ if (!err || strstr (err, "memsize") == NULL) {
+ fprintf (stderr, "invalid error message: %s\n", err ? err :
"NULL");
+ pthread_exit ((void *)-1);
+ }
+ }
+
+ pthread_exit (NULL);
+}
--
2.3.1
Daniel P. Berrange
2015-Jun-11 10:27 UTC
Re: [Libguestfs] [PATCH 3/5] threads: Use thread-local storage for errors.
On Sat, Jun 06, 2015 at 02:20:39PM +0100, Richard W.M. Jones wrote:> We permit the following constructs in libguestfs code: > > if (guestfs_some_call (g) == -1) { > fprintf (stderr, "failed: error is %s\n", guestfs_last_error (g)); > } > > and: > > guestfs_push_error_handler (g, NULL, NULL); > guestfs_some_call (g); > guestfs_pop_error_handler (g); > > Neither of these would be safe if we allowed the handle to be used > from threads concurrently, since the error string or error handler > could be changed by another thread. > > Solve this in approximately the same way that libvirt does: by making > the error, current error handler, and stack of error handlers use > thread-local storage (TLS). > > The implementation is not entirely straightforward, mainly because > POSIX doesn't give us useful destructor behaviour, so effectively we > end up creating our own destructor using a linked list. > > Note that you have to set the error handler in each thread separately, > which is an API change (eg: if you set the error handler in one > thread, then pass the handle 'g' to another thread, the error handler > in the second thread appears to have reset itself back to the default > error handler). I haven't yet worked out a better way to solve this.Do you have a feeling whether the error handler function push/pop is a commonly used feature or not ? In libvirt we originally had virGetLastError (global error) virConnGetLastError (per connection error) virSetErrorFunc (global error handler func) When we introduced thread support we didn't try to make the virConnGetLastError/SetErrorFun work. We just updated the docs to tell applications to /never/ use them in threaded applications, and always use the virGetLastError which was thread-local backed. For back compatibility we do copy the error from the global thread local into the per-connection object, so we didn't break single-threaded apps using that old function. IOW I think you could avoid alot of the complexity here if you just marked the existing error handler functions as deprecated in the context of multi-threaded usage and just introduced a single global "get error" function that was backed by a static allocated thread local. Regards, 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 :|
Pino Toscano
2015-Jun-11 13:13 UTC
Re: [Libguestfs] [PATCH 2/5] threads: Acquire and release the lock around each public guestfs_* API.
Hi, On Saturday 06 June 2015 14:20:38 Richard W.M. Jones wrote:> Since each ACQUIRE_LOCK/RELEASE_LOCK call must balance, this code is > difficult to debug. Enable DEBUG_LOCK to add some prints which can > help.There's some way this could be simplified:> const char * > guestfs_last_error (guestfs_h *g) > { > - return g->last_error; > + const char *r; > + > + ACQUIRE_LOCK (g); > + r = unlocked_last_error (g); > + RELEASE_LOCK (g); > + return r; > +}(picking this because it is the first ACQUIRE_LOCK ... RELEASE_LOCK pattern in the patch) there could be an extra macro to do a "stack lock", using the cleanup attribute we use already; something like: static inline guestfs_h *acquire_lock (guestfs_h *g) { ACQUIRE_LOCK (g); return g; } static inline void cleanup_release_lock (void *ptr) { guestfs_h *g = * (guestfs_h **) ptr; RELEASE_LOCK (g); } #define STACK_LOCK (g) \ __attribute__((cleanup(cleanup_release_lock))) \ guestfs_h *_stack_lock_ = acquire_lock (g) then the function above could look like: const char * guestfs_last_error (guestfs_h *g) { STACK_LOCK (g); return g->last_error; } This would have the drawback of requiring __attribute__((cleanup)) to work, although that could mean we could drop the code handling its lack. -- Pino Toscano
Reasonably Related Threads
- [PATCH threads v2 0/5] Add support for thread-safe handle.
- [PATCH v3 0/5] threads: Add support for thread-safe handle.
- [PATCH v3 REPOST 0/5] threads: Add support for thread-safe handle.
- [PATCH 2/5] threads: Acquire and release the lock around each public guestfs_* API.
- [PATCH 3/5] threads: Use thread-local storage for errors.