[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