[Libguestfs] [PATCH 2/5] threads: Acquire and release the lock around each public guestfs_* API.

Richard W.M. Jones rjones at redhat.com
Sat Jun 6 13:20:38 UTC 2015


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




More information about the Libguestfs mailing list