Libguestfs - Jul 2017 - Re: [PATCH v3 0/5] threads: Add support for thread-safe handle.

Previously posted in 2015:
v1: https://www.redhat.com/archives/libguestfs/2015-June/msg00048.html
v2: https://www.redhat.com/archives/libguestfs/2015-June/msg00118.html

I have rebased and tidied up the patches, fixing a few spelling
mistakes, but they are broadly the same as before.  I also ran all the
tests, which pass.

As with the previous versions, this makes a change to the API, where
you can no longer pass a handle between threads and expect
guestfs_last_error() to work.  I'm somewhat more sanguine about this
change, since using the API like that is abstruse and no one should be
expecting that to have worked.

Rich.

Add a g->lock field.  This commit simply initializes and destroys the
lock on handle creation/free, and does nothing else.
---
 lib/guestfs-internal.h | 6 ++++++
 lib/handle.c           | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index ec70336e2..c4a14e962 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -53,6 +53,7 @@
 #endif
 #endif
 
+#include "glthread/lock.h"
 #include "hash.h"
 
 #include "guestfs-internal-frontend.h"
@@ -389,6 +390,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
+   * protect the handle.
+   */
+  gl_recursive_lock_define (, lock);
+
   /**** Configuration of the handle. ****/
   bool verbose;                 /* Debugging. */
   bool trace;                   /* Trace calls. */
diff --git a/lib/handle.c b/lib/handle.c
index 91f5f755d..09c29ed84 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -86,6 +86,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;
@@ -169,6 +171,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;
 }
@@ -399,6 +402,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.13.0

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

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

---
 lib/guestfs.pod | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/lib/guestfs.pod b/lib/guestfs.pod
index f2a54a1fd..c337345f0 100644
--- a/lib/guestfs.pod
+++ b/lib/guestfs.pod
@@ -1338,14 +1338,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.38
+
+In libguestfs E<ge> 1.38, 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.38, 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.38
+
+In libguestfs E<lt> 1.38, 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.38.
 
 Use L</guestfs_set_identifier> to make it simpler to identify threads
 in trace output.
-- 
2.13.0

---
 .gitignore                 |   1 +
 tests/c-api/Makefile.am    |  21 ++++++-
 tests/c-api/test-threads.c | 133 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index 7ccc928bb..cd1351e41 100644
--- a/.gitignore
+++ b/.gitignore
@@ -552,6 +552,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 cb653f7a6..fe5736bd4 100644
--- a/tests/c-api/Makefile.am
+++ b/tests/c-api/Makefile.am
@@ -41,7 +41,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
@@ -59,7 +60,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
@@ -263,6 +265,21 @@ test_event_string_LDADD = \
 	$(LTLIBINTL) \
 	$(top_builddir)/gnulib/lib/libgnu.la
 
+test_threads_SOURCES = test-threads.c
+test_threads_CPPFLAGS = \
+	-I$(top_srcdir)/common/utils -I$(top_builddir)/common/utils \
+	-I$(top_srcdir)/lib -I$(top_builddir)/lib \
+	-I$(top_srcdir)/gnulib/lib \
+	-I$(top_builddir)/gnulib/lib
+test_threads_CFLAGS = \
+	-pthread \
+	$(WARN_CFLAGS) $(WERROR_CFLAGS)
+test_threads_LDADD = \
+	$(top_builddir)/lib/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 000000000..da39b7d41
--- /dev/null
+++ b/tests/c-api/test-threads.c
@@ -0,0 +1,133 @@
+/* libguestfs
+ * Copyright (C) 2015-2017 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.13.0

On Tue, Jun 27, 2017 at 12:55:54PM +0100, Richard W.M. Jones
wrote:
> Previously posted in 2015:
> v1: https://www.redhat.com/archives/libguestfs/2015-June/msg00048.html
> v2: https://www.redhat.com/archives/libguestfs/2015-June/msg00118.html
> 
> I have rebased and tidied up the patches, fixing a few spelling
> mistakes, but they are broadly the same as before.  I also ran all the
> tests, which pass.
> 
> As with the previous versions, this makes a change to the API, where
> you can no longer pass a handle between threads and expect
> guestfs_last_error() to work.  I'm somewhat more sanguine about this
> change, since using the API like that is abstruse and no one should be
> expecting that to have worked.
Yeah, I think it is reasonable to expect that any use of guestfs_last_error
should have been done immediately after the API call that failed, and
in the same thread.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Tuesday, 27 June 2017 13:55:57 CEST 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.
I'm not sure which behaviour you are referring to, but it should work
just fine -- in the destructor function, cast the void* argument to the
error_data struct, and free the linked list associated.

The only problem is that the tls gnulib implementation on Windows
(actually, on mingw) makes no use for the destructor function, but
a) we don't have a working Windows port ATM (not even on other
   non-Linux Unices)
b) using pthread or pth on mingw would solve this

I'd just use the destructor function approach, so there is less
complexity involved, and the per-thread resources are properly free'd
on thread exit (and not piling up until the handle destruction).

-- 
Pino Toscano

On Tuesday, 27 June 2017 13:55:54 CEST Richard W.M. Jones
wrote:
> Previously posted in 2015:
> v1: https://www.redhat.com/archives/libguestfs/2015-June/msg00048.html
> v2: https://www.redhat.com/archives/libguestfs/2015-June/msg00118.html
> 
> I have rebased and tidied up the patches, fixing a few spelling
> mistakes, but they are broadly the same as before.  I also ran all the
> tests, which pass.
> 
> As with the previous versions, this makes a change to the API, where
> you can no longer pass a handle between threads and expect
> guestfs_last_error() to work.  I'm somewhat more sanguine about this
> change, since using the API like that is abstruse and no one should be
> expecting that to have worked.
A general question on this: does enabling locking for all the handle
operation cause any noticeable performance issue in single-thread
handle usages?
Would it be worth making it opt-in somehow, so only users of the API
that require this kind of thread-safety would enable it?

-- 
Pino Toscano

On Mon, Jul 10, 2017 at 01:52:59PM +0200, Pino Toscano
wrote:
> On Tuesday, 27 June 2017 13:55:54 CEST Richard W.M. Jones wrote:
> > Previously posted in 2015:
> > v1: https://www.redhat.com/archives/libguestfs/2015-June/msg00048.html
> > v2: https://www.redhat.com/archives/libguestfs/2015-June/msg00118.html
> > 
> > I have rebased and tidied up the patches, fixing a few spelling
> > mistakes, but they are broadly the same as before.  I also ran all the
> > tests, which pass.
> > 
> > As with the previous versions, this makes a change to the API, where
> > you can no longer pass a handle between threads and expect
> > guestfs_last_error() to work.  I'm somewhat more sanguine about
this
> > change, since using the API like that is abstruse and no one should be
> > expecting that to have worked.
> 
> A general question on this: does enabling locking for all the handle
> operation cause any noticeable performance issue in single-thread
> handle usages?
Because we're using the gnulib wrapper, if the program is not linked
to pthread then TLS is not used at all.  Instead a simple pointer is
substituted with the key.

However this is no longer a very realistic scenario.  You would have
to disable libvirt and probably other libraries, to be sure that
pthread is not being linked to your libguestfs-using program.
> Would it be worth making it opt-in somehow, so only users of the API
> that require this kind of thread-safety would enable it?
I would be surprised if it was noticable.  The overhead of calling
guestfs_set_verbose just to set an integer flag is already large, see
below.  I think making the API usable from threads without requiring
any thought is better.

Rich.

GUESTFS_DLL_PUBLIC int
guestfs_set_verbose (guestfs_h *g,
                     int verbose)
{
  int trace_flag = g->trace;
  struct trace_buffer trace_buffer;
  int r;

  // This function iterates i = 0 .. nr_events-1.
  guestfs_int_call_callbacks_message (g, GUESTFS_EVENT_ENTER,
                                      "set_verbose", 11);
  if (trace_flag) {
    guestfs_int_trace_open (&trace_buffer);
    fprintf (trace_buffer.fp, "%s", "set_verbose");
    fputs (verbose ? " true" : " false", trace_buffer.fp);
    guestfs_int_trace_send_line (g, &trace_buffer);
  }

  r = guestfs_impl_set_verbose (g, verbose);

  if (r != -1) {
    if (trace_flag) {
      guestfs_int_trace_open (&trace_buffer);
      fprintf (trace_buffer.fp, "%s = ", "set_verbose");
      fprintf (trace_buffer.fp, "%d", r);
      guestfs_int_trace_send_line (g, &trace_buffer);
    }

  } else {
    if (trace_flag)
      guestfs_int_trace (g, "%s = %s (error)",
                         "set_verbose", "-1");
  }

  return r;
}



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
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/