[Libguestfs] [PATCH nbdkit 1/2] connections: Protect open and close callbacks with the request lock.

[Richard W.M. Jones]

The thread model SERIALIZE_ALL_REQUESTS claims that although multiple
handles may be open at the same time,

    "[...] data requests
     are serialized so that for the plugin as a whole only one
     read/write/etc request will be in progress at any time."

The text seems to apply to only datapath requests but goes on to say:

    "This is a useful setting if the library you are using is not
     thread-safe.  However performance may not be good."

If the plugin calls into a non-thread-safe library during open() or
close() (as the Python plugin does) then you are not protected.

We could change every plugin, but it seems better to extend the
protection of SERIALIZE_ALL_REQUESTS so it also applies to open() and
close() callbacks, to reduce confusion.

This fixes two problems in the Python plugin if the user-defined
close() callback takes a long time to run (for example it is doing
cleanups).  See:

https://bugzilla.redhat.com/show_bug.cgi?id=1566516
https://bugzilla.redhat.com/show_bug.cgi?id=1566522
---
 docs/nbdkit-plugin.pod |  6 +++---
 src/connections.c      | 28 +++++++++++++++++-----------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 6e89315..672f02a 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -663,9 +663,9 @@ first client to close the connection.
 
 I<This is a safe default for most plugins>.
 
-Multiple handles can be open at the same time, but data requests are
-serialized so that for the plugin as a whole only one read/write/etc
-request will be in progress at any time.
+Multiple handles can be open at the same time, but requests are
+serialized so that for the plugin as a whole only one
+open/read/write/close (etc) request will be in progress at any time.
 
 This is a useful setting if the library you are using is not
 thread-safe.  However performance may not be good.
diff --git a/src/connections.c b/src/connections.c
index fbcf5e5..5024e2f 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -233,7 +233,7 @@ connection_worker (void *data)
 static int
 _handle_single_connection (int sockin, int sockout)
 {
-  int r = -1;
+  int ret = -1, r;
   struct connection *conn;
   int nworkers = threads ? threads : DEFAULT_PARALLEL_REQUESTS;
   pthread_t *workers = NULL;
@@ -245,7 +245,10 @@ _handle_single_connection (int sockin, int sockout)
   if (!conn)
     goto done;
 
-  if (backend->open (backend, conn, readonly) == -1)
+  lock_request (conn);
+  r = backend->open (backend, conn, readonly);
+  unlock_request (conn);
+  if (r == -1)
     goto done;
 
   threadlocal_set_name (backend->plugin_name (backend));
@@ -312,11 +315,11 @@ _handle_single_connection (int sockin, int sockout)
   if (finalize (conn) == -1)
     goto done;
 
-  r = get_status (conn);
+  ret = get_status (conn);
  done:
-  debug ("connection cleanup with final status %d", r);
+  debug ("connection cleanup with final status %d", ret);
   free_connection (conn);
-  return r;
+  return ret;
 }
 
 int
@@ -366,20 +369,23 @@ free_connection (struct connection *conn)
 
   conn->close (conn);
 
-  pthread_mutex_destroy (&conn->request_lock);
-  pthread_mutex_destroy (&conn->read_lock);
-  pthread_mutex_destroy (&conn->write_lock);
-  pthread_mutex_destroy (&conn->status_lock);
-
   /* Don't call the plugin again if quit has been set because the main
    * thread will be in the process of unloading it.  The plugin.unload
    * callback should always be called.
    */
   if (!quit) {
-    if (conn->nr_handles > 0 && conn->handles[0])
+    if (conn->nr_handles > 0 && conn->handles[0]) {
+      lock_request (conn);
       backend->close (backend, conn);
+      unlock_request (conn);
+    }
   }
 
+  pthread_mutex_destroy (&conn->request_lock);
+  pthread_mutex_destroy (&conn->read_lock);
+  pthread_mutex_destroy (&conn->write_lock);
+  pthread_mutex_destroy (&conn->status_lock);
+
   free (conn->handles);
   free (conn);
 }
-- 
2.16.2