[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[Libguestfs] [PATCH v3 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 +
 lib/errors.c           | 196 ++++++++++++++++++++++++++++++++++++++++---------
 lib/guestfs-internal.h |  25 +++----
 lib/handle.c           |  11 +--
 m4/.gitignore          |   1 +
 5 files changed, 180 insertions(+), 54 deletions(-)

diff --git a/bootstrap b/bootstrap
index 77a95a25b..4e3d4bc51 100755
--- a/bootstrap
+++ b/bootstrap
@@ -95,6 +95,7 @@ symlinkat
 sys_select
 sys_types
 sys_wait
+tls
 vasprintf
 vc-list-files
 warnings
diff --git a/lib/errors.c b/lib/errors.c
index def1d3c89..b0c678d77 100644
--- a/lib/errors.c
+++ b/lib/errors.c
@@ -51,26 +51,150 @@
 #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_delete
+ * 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;
+}
+
 const char *
 guestfs_last_error (guestfs_h *g)
 {
   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
-  return g->last_error;
+  return get_error_data (g)->last_error;
 }
 
 int
 guestfs_last_errno (guestfs_h *g)
 {
   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
-  return g->last_errnum;
+  return get_error_data (g)->last_errnum;
 }
 
 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;
 }
 
 /**
@@ -166,6 +290,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);
@@ -177,7 +302,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);
 }
 
 /**
@@ -196,6 +322,7 @@ guestfs_int_perrorf (guestfs_h *g, const char *fs, ...)
   const 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);
@@ -213,7 +340,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
@@ -235,16 +363,21 @@ 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;
+  struct error_data *error_data;
+
+  error_data = get_error_data (g);
+  error_data->error_cb = cb;
+  error_data->error_cb_data = data;
 }
 
 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;
+  struct error_data *error_data = get_error_data (g);
+
+  if (data_rtn) *data_rtn = error_data->error_cb_data;
+  return error_data->error_cb;
 }
 
 void
@@ -252,13 +385,15 @@ guestfs_push_error_handler (guestfs_h *g,
                             guestfs_error_handler_cb cb, void *data)
 {
   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
+  struct error_data *error_data;
   struct error_cb_stack *old_stack;
 
-  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);
 }
@@ -267,26 +402,21 @@ void
 guestfs_pop_error_handler (guestfs_h *g)
 {
   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (g);
+  struct error_data *error_data;
   struct error_cb_stack *next_stack;
 
-  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);
-}
-
-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
diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 37878b88d..c0a244a1e 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -54,6 +54,7 @@
 #endif
 
 #include "glthread/lock.h"
+#include "glthread/tls.h"
 #include "hash.h"
 
 #include "guestfs-internal-frontend.h"
@@ -373,15 +374,6 @@ struct connection_ops {
 };
 
 /**
- * Stack of old error handlers.
- */
-struct error_cb_stack {
-  struct error_cb_stack   *next;
-  guestfs_error_handler_cb error_cb;
-  void *                   error_cb_data;
-};
-
-/**
  * Cache of queried features.
  *
  * Used to cache the appliance features (see F<lib/available.c>).
@@ -458,9 +450,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.
@@ -474,9 +463,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;
@@ -706,7 +699,7 @@ extern char *guestfs_int_safe_asprintf (guestfs_h *g, const char *fs, ...)
 #define safe_asprintf guestfs_int_safe_asprintf
 
 /* 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/lib/handle.c b/lib/handle.c
index 183f247fb..7f528c129 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -32,6 +32,7 @@
 #include <libxml/xmlversion.h>
 
 #include "glthread/lock.h"
+#include "glthread/tls.h"
 #include "ignore-value.h"
 #include "c-ctype.h"
 #include "getprogname.h"
@@ -92,7 +93,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;
@@ -171,6 +172,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;
@@ -383,9 +386,6 @@ 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);
@@ -394,7 +394,6 @@ guestfs_close (guestfs_h *g)
   free (g->env_runtimedir);
   free (g->int_tmpdir);
   free (g->int_cachedir);
-  free (g->last_error);
   free (g->identifier);
   free (g->program);
   free (g->path);
@@ -403,6 +402,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
diff --git a/m4/.gitignore b/m4/.gitignore
index 07960ed7b..a84b22e5c 100644
--- a/m4/.gitignore
+++ b/m4/.gitignore
@@ -248,6 +248,7 @@
 /thread.m4
 /time_h.m4
 /timespec.m4
+/tls.m4
 /ttyname_r.m4
 /ulonglong.m4
 /ungetc.m4
-- 
2.13.0


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]