Libguestfs - Jul 2019 - [PATCH libnbd v3 2/2] lib: Remove nbd_add_close_callback.

I think I've addressed everything that was raised in review.
Some of the highlights:

 - Callbacks should be freed reliably along all exit paths.

 - There's a simple test of closure lifetimes.

 - I've tried to use VALID|FREE in all the places where I'm confident
   that it's safe and correct to do.  There may be more places.  Note
   this is an optimization and shouldn't affect correctness.

 - It's a bit difficult to use VALID|FREE when freeing the debug
   callback (because context is not easily available) so I didn't make
   that change.

Rich.

Previously closures had a crude flag which tells if they are
persistent or transient.  Transient closures (flag = false) last for
the lifetime of the currently called libnbd function.  Persistent
closures had an indefinite lifetime which could last for as long as
the handle.  In language bindings handling persistent closures was
wasteful as we needed to register a "close callback" to free the
closure when the handle is closed.  But if you had submitted thousands
of asynchronous commands you would end up registering thousands of
close callbacks.

We actually have far more information about the lifetime of closures.
We know precisely when they will no longer be needed by the library.
Callers can use this information to free up associated user data
earlier.  In particular in language bindings we can remove roots /
decrement reference counts at the right place to free the closure,
without waiting for the handle to be closed.

The solution to this is to introduce the concept of a closure
lifetime.  The callback is called with an extra valid_flag parameter
which is a bitmap containing LIBNBD_CALLBACK_VALID and/or
LIBNBD_CALLBACK_FREE.  LIBNBD_CALLBACK_VALID corresponds to a normal
call of the callback function by the library.  After the library has
finished with the callback (declaring that this callback will never be
needed or called again), it is called once more with
valid_flag == LIBNBD_CALLBACK_FREE.

(Note it is also possible for the library to call the callback with
valid_flag == LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, meaning it's the
last valid call.)

As an example, nbd_set_debug_callback registers a debug closure which
is saved in the handle.  The closure is freed either when
nbd_set_debug_callback is called again, or the handle is closed.  The
sequence of events would look like this:

  Caller                 Callback

  nbd_set_debug_callback

                         # a new debug_fn is registered
  calls any nbd function debug_fn (valid_flag == VALID)
                         debug_fn (valid_flag == VALID)
                         debug_fn (valid_flag == VALID)
                         ...

  nbd_set_debug_callback
                         # the old debug_fn is freed
                         debug_fn (valid_flag == FREE)

                         # the new debug_fn is called
  calls any nbd function debug_fn (valid_flag == VALID)
                         debug_fn (valid_flag == VALID)

                         # the second debug_fn is freed
  nbd_close              debug_fn (valid_flag == FREE)

An example of a debug_fn written by a caller would be:

 int my_debug_fn (int valid_flag, void *user_data,
                  const char *context, const char *msg)
 {
   if (valid_flag & LIBNBD_CALLBACK_VALID) {
     printf ("context = %s, msg = %s\n", context, msg);
   }
   if (valid_flag & LIBNBD_CALLBACK_FREE) {
     /* If you need to free something, for example: */
     free (user_data);
   }
   return 0;
 }
---
 .gitignore                          |   1 +
 docs/libnbd.pod                     |  59 +++++++-
 examples/glib-main-loop.c           |  16 ++-
 examples/strict-structured-reads.c  |  19 ++-
 generator/generator                 | 212 +++++++++++-----------------
 generator/states-reply-simple.c     |   5 +-
 generator/states-reply-structured.c |   8 +-
 generator/states-reply.c            |   8 +-
 generator/states.c                  |   8 +-
 interop/dirty-bitmap.c              |   5 +-
 interop/structured-read.c           |   6 +-
 lib/aio.c                           |  20 ++-
 lib/debug.c                         |   8 +-
 lib/handle.c                        |   6 +-
 lib/internal.h                      |  14 +-
 tests/Makefile.am                   |   7 +
 tests/closure-lifetimes.c           | 136 ++++++++++++++++++
 tests/meta-base-allocation.c        |  13 +-
 tests/oldstyle.c                    |  10 +-
 tests/server-death.c                |   5 +-
 20 files changed, 401 insertions(+), 165 deletions(-)

diff --git a/.gitignore b/.gitignore
index 9a8ba37..ed1e03e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -108,6 +108,7 @@ Makefile.in
 /tests/can-trim-flag
 /tests/can-not-trim-flag
 /tests/can-zero-flag
+/tests/closure-lifetimes
 /tests/connect-tcp
 /tests/connect-tls-certs
 /tests/connect-tls-psk
diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index 4d31c64..1924305 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -487,7 +487,64 @@ C<nbd_set_debug_callback>,
C<nbd_pread_callback>).  Libnbd can call
 these functions while processing.
 
 Callbacks have an opaque C<void *user_data> pointer.  This is passed
-as the first parameter to the callback.
+as the second parameter to the callback.  The opaque pointer is only
+used from the C API, since in other languages you can use closures to
+achieve the same outcome.
+
+=head2 Callback lifetimes
+
+All callbacks have an C<unsigned valid_flag> parameter which is used
+to help with the lifetime of the callback.  C<valid_flag> contains the
+I<logical or> of:
+
+=over 4
+
+=item C<LIBNBD_CALLBACK_VALID>
+
+The callback parameters are valid and this is a normal callback.
+
+=item C<LIBNBD_CALLBACK_FREE>
+
+This is the last time the library will call this function.  Any data
+associated with the callback can be freed.
+
+=item other bits
+
+Other bits in C<valid_flag> should be ignored.
+
+=back
+
+For example C<nbd_set_debug_callback> sets up a callback which you
+could define like this:
+
+ int my_debug_fn (unsigned valid_flag, void *user_data,
+                  const char *context, const char *msg)
+ {
+   if (valid_flag & LIBNBD_CALLBACK_VALID) {
+     printf ("context = %s, msg = %s\n", context, msg);
+   }
+   if (valid_flag & LIBNBD_CALLBACK_FREE) {
+     /* If you need to free something, for example: */
+     free (user_data);
+   }
+   return 0;
+ }
+
+If you don't care about the freeing feature then it is also correct to
+write this:
+
+ int my_debug_fn (unsigned valid_flag, void *user_data,
+                  const char *context, const char *msg)
+ {
+   if (!(valid_flag & LIBNBD_CALLBACK_VALID)) return 0;
+
+   // Rest of callback as normal.
+ }
+
+The valid flag is only present in the C API.  It is not needed when
+using garbage-collected programming languages.
+
+=head2 Callbacks and locking
 
 The callbacks are invoked at a point where the libnbd lock is held; as
 such, it is unsafe for the callback to call any C<nbd_*> APIs on the
diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c
index 2230077..2d192e6 100644
--- a/examples/glib-main-loop.c
+++ b/examples/glib-main-loop.c
@@ -274,9 +274,11 @@ static GMainLoop *loop;
 
 static void connected (struct NBDSource *source);
 static gboolean read_data (gpointer user_data);
-static int finished_read (void *vp, int64_t rcookie, int *error);
+static int finished_read (unsigned valid_flag, void *vp,
+                          int64_t rcookie, int *error);
 static gboolean write_data (gpointer user_data);
-static int finished_write (void *vp, int64_t wcookie, int *error);
+static int finished_write (unsigned valid_flag, void *vp,
+                           int64_t wcookie, int *error);
 
 int
 main (int argc, char *argv[])
@@ -397,10 +399,13 @@ read_data (gpointer user_data)
 
 /* This callback is called from libnbd when any read command finishes. */
 static int
-finished_read (void *vp, int64_t rcookie, int *error)
+finished_read (unsigned valid_flag, void *vp, int64_t rcookie, int *error)
 {
   size_t i;
 
+  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+    return 0;
+
   if (gssrc == NULL)
     return 0;
 
@@ -462,10 +467,13 @@ write_data (gpointer user_data)
 
 /* This callback is called from libnbd when any write command finishes. */
 static int
-finished_write (void *vp, int64_t wcookie, int *error)
+finished_write (unsigned valid_flag, void *vp, int64_t wcookie, int *error)
 {
   size_t i;
 
+  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+    return 0;
+
   if (gsdest == NULL)
     return 0;
 
diff --git a/examples/strict-structured-reads.c
b/examples/strict-structured-reads.c
index 1a551a0..511dd7c 100644
--- a/examples/strict-structured-reads.c
+++ b/examples/strict-structured-reads.c
@@ -51,12 +51,16 @@ static int64_t total_bytes;
 static int total_success;
 
 static int
-read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset,
+read_chunk (unsigned valid_flag, void *opaque,
+            const void *bufv, size_t count, uint64_t offset,
             unsigned status, int *error)
 {
   struct data *data = opaque;
   struct range *r, **prev;
 
+  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+    return 0;
+
   /* libnbd guarantees this: */
   assert (offset >= data->offset);
   assert (offset + count <= data->offset + data->count);
@@ -127,11 +131,14 @@ read_chunk (void *opaque, const void *bufv, size_t count,
uint64_t offset,
 }
 
 static int
-read_verify (void *opaque, int64_t cookie, int *error)
+read_verify (unsigned valid_flag, void *opaque, int64_t cookie, int *error)
 {
+  int ret = 0;
+
+  if (valid_flag & LIBNBD_CALLBACK_VALID) {
   struct data *data = opaque;
-  int ret = -1;
 
+    ret = -1;
   total_reads++;
   total_chunks += data->chunks;
   if (*error)
@@ -160,7 +167,11 @@ read_verify (void *opaque, int64_t cookie, int *error)
     data->remaining = r->next;
     free (r);
   }
-  free (data);
+  }
+
+  if (valid_flag & LIBNBD_CALLBACK_FREE)
+    free (opaque);
+
   return ret;
 }
 
diff --git a/generator/generator b/generator/generator
index b0ed959..7aad57c 100755
--- a/generator/generator
+++ b/generator/generator
@@ -849,10 +849,7 @@ and arg                                written by the
function *)
 | BytesPersistIn of string * string (* same as above, but buffer persists *)
 | BytesPersistOut of string * string
-| Closure of bool * closure (* function pointer + void *opaque
-                              flag if true means callbacks persist
-                              in the handle, false means they only
-                              exist during the function call *)
+| Closure of closure       (* function pointer + void *opaque *)
 | Flags of string          (* NBD_CMD_FLAG_* flags *)
 | Int of string            (* small int *)
 | Int64 of string          (* 64 bit signed int *)
@@ -920,9 +917,8 @@ Return the state of the debug flag on this handle.";
 
   "set_debug_callback", {
     default_call with
-    args = [ Closure (true,
-                      { cbname="debug_fn";
-                        cbargs=[String "context"; String
"msg"] }) ];
+    args = [ Closure { cbname="debug_fn";
+                       cbargs=[String "context"; String
"msg"] } ];
     ret = RErr;
     shortdesc = "set the debug callback";
     longdesc = "\
@@ -1350,11 +1346,10 @@ protocol extensions).";
   "pread_structured", {
     default_call with
     args = [ BytesOut ("buf", "count"); UInt64
"offset";
-             Closure (false,
-                      { cbname="chunk";
+             Closure { cbname="chunk";
                         cbargs=[BytesIn ("subbuf",
"count");
                                 UInt64 "offset"; UInt
"status";
-                                Mutable (Int "error")] });
+                                Mutable (Int "error")] };
              Flags "flags" ];
     ret = RErr;
     permitted_states = [ Connected ];
@@ -1541,13 +1536,12 @@ punching a hole.";
   "block_status", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Closure (false,
-                      { cbname="extent";
+             Closure { cbname="extent";
                         cbargs=[String "metacontext";
                                 UInt64 "offset";
                                 ArrayAndLen (UInt32 "entries",
                                              "nr_entries");
-                                Mutable (Int "error")]} );
+                               Mutable (Int "error")]};
              Flags "flags" ];
     ret = RErr;
     permitted_states = [ Connected ];
@@ -1725,9 +1719,8 @@ C<nbd_pread>.";
   "aio_pread_callback", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Closure (true,
-                      { cbname="callback";
-                        cbargs=[Int64 "cookie"; Mutable (Int
"error")] } );
+             Closure { cbname="callback";
+                       cbargs=[Int64 "cookie"; Mutable (Int
"error")] };
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1744,12 +1737,11 @@ completed.  Other parameters behave as documented in
C<nbd_pread>.";
   "aio_pread_structured", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Closure (true,
-                      { cbname="chunk";
+             Closure { cbname="chunk";
                         cbargs=[BytesIn ("subbuf",
"count");
                                 UInt64 "offset";
                                 UInt "status";
-                                Mutable (Int "error");]} );
+                                Mutable (Int "error");]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1766,16 +1758,14 @@ documented in C<nbd_pread_structured>.";
   "aio_pread_structured_callback", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Closure (true,
-                      { cbname="chunk";
+             Closure { cbname="chunk";
                         cbargs=[BytesIn ("subbuf",
"count");
                                 UInt64 "offset";
                                 UInt "status";
-                                Mutable (Int "error"); ]});
-             Closure (true,
-                      { cbname="callback";
-                        cbargs=[Int64 "cookie";
-                                Mutable (Int "error"); ]} );
+                                Mutable (Int "error"); ]};
+             Closure { cbname="callback";
+                       cbargs=[Int64 "cookie";
+                               Mutable (Int "error"); ]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1807,9 +1797,8 @@ C<nbd_pwrite>.";
   "aio_pwrite_callback", {
     default_call with
     args = [ BytesPersistIn ("buf", "count"); UInt64
"offset";
-             Closure (true,
-                      { cbname="callback";
-                        cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
+             Closure { cbname="callback";
+                       cbargs=[Int64 "cookie"; Mutable (Int
"error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1861,9 +1850,8 @@ Parameters behave as documented in
C<nbd_flush>.";
 
   "aio_flush_callback", {
     default_call with
-    args = [ Closure (true,
-                      { cbname="callback";
-                        cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
+    args = [ Closure { cbname="callback";
+                       cbargs=[Int64 "cookie"; Mutable (Int
"error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1893,9 +1881,8 @@ Parameters behave as documented in
C<nbd_trim>.";
   "aio_trim_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Closure (true,
-                      { cbname="callback";
-                        cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
+             Closure { cbname="callback";
+                       cbargs=[Int64 "cookie"; Mutable (Int
"error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1925,9 +1912,8 @@ Parameters behave as documented in
C<nbd_cache>.";
   "aio_cache_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Closure (true,
-                      { cbname="callback";
-                        cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
+             Closure { cbname="callback";
+                       cbargs=[Int64 "cookie"; Mutable (Int
"error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1957,9 +1943,8 @@ Parameters behave as documented in
C<nbd_zero>.";
   "aio_zero_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Closure (true,
-                      { cbname="callback";
-                        cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
+             Closure { cbname="callback";
+                       cbargs=[Int64 "cookie"; Mutable (Int
"error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1975,12 +1960,11 @@ Other parameters behave as documented in
C<nbd_zero>.";
   "aio_block_status", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Closure (true,
-                      { cbname="extent";
+             Closure { cbname="extent";
                         cbargs=[String "metacontext"; UInt64
"offset";
                                 ArrayAndLen (UInt32 "entries",
                                              "nr_entries");
-                                Mutable (Int "error")] } );
+                               Mutable (Int "error")] };
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1996,15 +1980,13 @@ Parameters behave as documented in
C<nbd_block_status>.";
   "aio_block_status_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Closure (true,
-                      { cbname="extent";
+             Closure { cbname="extent";
                         cbargs=[String "metacontext"; UInt64
"offset";
                                 ArrayAndLen (UInt32 "entries",
                                              "nr_entries");
-                                Mutable (Int "error")]});
-             Closure (true,
-                      { cbname="callback";
-                        cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
+                               Mutable (Int "error")]};
+             Closure { cbname="callback";
+                       cbargs=[Int64 "cookie"; Mutable (Int
"error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -3037,7 +3019,8 @@ module C : sig
   val generate_lib_unlocked_h : unit -> unit
   val generate_lib_api_c : unit -> unit
   val generate_docs_libnbd_api_pod : unit -> unit
-  val print_arg_list : ?handle:bool -> ?user_data:bool -> ?types:bool
-> arg list -> unit
+  val print_arg_list : ?handle:bool -> ?valid_flag:bool ->
?user_data:bool ->
+                       ?types:bool -> arg list -> unit
 end = struct
 
 (* Check the API definition. *)
@@ -3090,7 +3073,7 @@ let rec name_of_arg = function
 | BytesOut (n, len) -> [n; len]
 | BytesPersistIn (n, len) -> [n; len]
 | BytesPersistOut (n, len) -> [n; len]
-| Closure (_, { cbname }) -> [cbname; sprintf "%s_user_data"
cbname ]
+| Closure { cbname } -> [cbname; sprintf "%s_user_data" cbname ]
 | Flags n -> [n]
 | Int n -> [n]
 | Int64 n -> [n]
@@ -3103,7 +3086,8 @@ let rec name_of_arg = function
 | UInt32 n -> [n]
 | UInt64 n -> [n]
 
-let rec print_arg_list ?(handle = false) ?(user_data = false)
+let rec print_arg_list ?(handle = false) ?(valid_flag = false)
+                       ?(user_data = false)
                        ?(types = true) args    pr "(";
   let comma = ref false in
@@ -3112,6 +3096,12 @@ let rec print_arg_list ?(handle = false) ?(user_data =
false)
     if types then pr "struct nbd_handle *";
     pr "h"
   );
+  if valid_flag then (
+    if !comma then pr ", ";
+    comma := true;
+    if types then pr "unsigned ";
+    pr "valid_flag";
+  );
   if user_data then (
     if !comma then pr ", ";
     comma := true;
@@ -3144,10 +3134,10 @@ let rec print_arg_list ?(handle = false) ?(user_data =
false)
          pr "%s, " n;
          if types then pr "size_t ";
          pr "%s" len
-      | Closure (_, { cbname; cbargs }) ->
+      | Closure { cbname; cbargs } ->
          if types then (
            pr "int (*%s) " cbname;
-           print_arg_list ~user_data:true cbargs;
+           print_arg_list ~valid_flag:true ~user_data:true cbargs;
          )
          else
            pr "%s" cbname;
@@ -3250,6 +3240,9 @@ let generate_include_libnbd_h ()    pr "\n";
   List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i)
constants;
   pr "\n";
+  pr "#define LIBNBD_CALLBACK_VALID 1\n";
+  pr "#define LIBNBD_CALLBACK_FREE  2\n";
+  pr "\n";
   pr "extern struct nbd_handle *nbd_create (void);\n";
   pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
   pr "\n";
@@ -3714,28 +3707,16 @@ let print_python_binding name { args; ret }     *)
   List.iter (
     function
-    | Closure (persistent, { cbname; cbargs }) ->
-       (* Persistent closures need an explicit function to decrement
-        * the closure refcounts and free the user_data struct.
-        *)
-       if persistent then (
-         pr "static void\n";
-         pr "free_%s_%s_user_data (void *vp)\n" name cbname;
-         pr "{\n";
-         pr "  PyObject *user_data = vp;\n";
-         pr "\n";
-         pr "  Py_DECREF (user_data);\n";
-         pr "}\n";
-         pr "\n";
-       );
-
+    | Closure { cbname; cbargs } ->
        pr "/* Wrapper for %s callback of %s. */\n" cbname name;
        pr "static int\n";
        pr "%s_%s_wrapper " name cbname;
-       C.print_arg_list ~user_data:true cbargs;
+       C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
        pr "\n";
        pr "{\n";
-       pr "  int ret;\n";
+       pr "  int ret = 0;\n";
+       pr "\n";
+       pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
        pr "  PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
        pr "  PyObject *py_args, *py_ret;\n";
        List.iter (
@@ -3818,7 +3799,6 @@ let print_python_binding name { args; ret }         pr
"  Py_DECREF (py_args);\n";
        pr "\n";
        pr "  if (py_ret != NULL) {\n";
-       pr "    ret = 0;\n";
        pr "    Py_DECREF (py_ret); /* return value is discarded
*/\n";
        pr "  }\n";
        pr "  else {\n";
@@ -3847,6 +3827,11 @@ let print_python_binding name { args; ret }           |
Path _ | SockAddrAndLen _ | StringList _
          | UInt32 _ -> assert false
        ) cbargs;
+       pr "  }\n";
+       pr "\n";
+       pr "  if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
+       pr "    Py_DECREF ((PyObject *)user_data);\n";
+       pr "\n";
        pr "  return ret;\n";
        pr "}\n";
        pr "\n"
@@ -3887,7 +3872,7 @@ let print_python_binding name { args; ret }         pr
"  PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n"
           n;
        pr "  struct py_aio_buffer *%s_buf;\n" n
-    | Closure (_, { cbname }) ->
+    | Closure { cbname } ->
        pr "  PyObject *%s_user_data;\n" cbname
     | Flags n ->
        pr "  uint32_t %s_u32;\n" n;
@@ -3953,7 +3938,7 @@ let print_python_binding name { args; ret }      | BytesIn
(n, _) | BytesPersistIn (n, _)
     | BytesPersistOut (n, _) -> pr ", &%s" n
     | BytesOut (_, count) -> pr ", &%s" count
-    | Closure (_, { cbname }) -> pr ", &%s_user_data" cbname
+    | Closure { cbname } -> pr ", &%s_user_data" cbname
     | Flags n -> pr ", &%s" n
     | Int n -> pr ", &%s" n
     | Int64 n -> pr ", &%s" n
@@ -3969,17 +3954,6 @@ let print_python_binding name { args; ret }    pr
"))\n";
   pr "    return NULL;\n";
 
-  (* If the parameter has persistent closures then we need to
-   * make sure the ref count remains positive.
-   *)
-  List.iter (
-    function
-    | Closure (false, _) -> ()
-    | Closure (true, { cbname }) ->
-       pr "  Py_INCREF (%s_user_data);\n" cbname
-    | _ -> ()
-  ) args;
-
   pr "  h = get_handle (py_h);\n";
   List.iter (
     function
@@ -4007,7 +3981,9 @@ let print_python_binding name { args; ret }         pr
"  %s = malloc (%s);\n" n count
     | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
        pr "  %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n
-    | Closure (_, { cbname }) ->
+    | Closure { cbname } ->
+       pr "  /* Increment refcount since pointer may be saved by libnbd.
*/\n";
+       pr "  Py_INCREF (%s_user_data);\n" cbname;
        pr "  if (!PyCallable_Check (%s_user_data)) {\n" cbname;
            pr "    PyErr_SetString (PyExc_TypeError,\n";
            pr "                     \"callback parameter %s is not
callable\");\n" cbname;
@@ -4043,7 +4019,7 @@ let print_python_binding name { args; ret }      |
BytesOut (n, count) -> pr ", %s, %s" n count
     | BytesPersistIn (n, _)
     | BytesPersistOut (n, _) -> pr ", %s_buf->data,
%s_buf->len" n n
-    | Closure (_, { cbname }) ->
+    | Closure { cbname } ->
        pr ", %s_%s_wrapper" name cbname;
        pr ", %s_user_data" cbname
     | Flags n -> pr ", %s_u32" n
@@ -4121,11 +4097,7 @@ let print_python_binding name { args; ret }      | Bool _
-> ()
     | BytesIn (n, _) -> pr "  PyBuffer_Release (&%s);\n" n
     | BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> ()
-    | Closure (false, _) -> ()
-    | Closure (true, { cbname }) ->
-       pr "  /* This ensures the callback data is freed eventually.
*/\n";
-       pr "  nbd_add_close_callback (h, free_%s_%s_user_data,
%s_user_data);\n"
-          name cbname cbname
+    | Closure _ -> ()
     | Flags _ -> ()
     | Int _ -> ()
     | Int64 _ -> ()
@@ -4272,7 +4244,7 @@ class NBD (object):
             | BytesIn (n, _) | BytesPersistIn (n, _) -> [n, None]
             | BytesPersistOut (n, _) -> [n, None]
             | BytesOut (_, count) -> [count, None]
-            | Closure (_, { cbname }) -> [cbname, None]
+            | Closure { cbname } -> [cbname, None]
             | Flags n -> [n, Some "0"]
             | Int n -> [n, None]
             | Int64 n -> [n, None]
@@ -4345,10 +4317,7 @@ let args_to_ocaml_args args      | Flags n :: rest ->
Some (OCamlFlags n), List.rev rest
     | _ -> None, args in
   let args -    List.map (
-      function
-      | a -> [OCamlArg a]
-    ) args in
+    List.map (fun a -> [OCamlArg a]) args in
   let args = List.flatten args in
   match flags with
   | Some f -> f :: OCamlHandle :: args
@@ -4371,7 +4340,7 @@ and ocaml_arg_to_string = function
   | OCamlArg (BytesPersistIn _) -> "Buffer.t"
   | OCamlArg (BytesOut _) -> "bytes"
   | OCamlArg (BytesPersistOut _) -> "Buffer.t"
-  | OCamlArg (Closure (_, { cbargs })) ->
+  | OCamlArg (Closure { cbargs }) ->
      sprintf "(%s)"
              (ocaml_fundecl_to_string (List.map (fun a -> OCamlArg a)
cbargs)
                                       RErr)
@@ -4407,7 +4376,7 @@ let rec name_of_ocaml_arg = function
      | BytesOut (n, len) -> n
      | BytesPersistIn (n, len) -> n
      | BytesPersistOut (n, len) -> n
-     | Closure (_, { cbname }) -> cbname
+     | Closure { cbname } -> cbname
      | Flags n -> n
      | Int n -> n
      | Int64 n -> n
@@ -4565,22 +4534,7 @@ let print_ocaml_binding (name, { args; ret })    (*
Functions with a callback parameter require special handling. *)
   List.iter (
     function
-    | Closure (persistent, { cbname; cbargs }) ->
-       (* Persistent closures need an explicit function to remove
-        * the global root and free the user_data struct.
-        *)
-       if persistent then (
-         pr "static void\n";
-         pr "free_%s_%s_user_data (void *vp)\n" name cbname;
-         pr "{\n";
-         pr "  value *user_data = vp;\n";
-         pr "\n";
-         pr "  caml_remove_generational_global_root (user_data);\n";
-         pr "  free (user_data);\n";
-         pr "}\n";
-         pr "\n"
-       );
-
+    | Closure { cbname; cbargs } ->
        let argnames           List.map (
            function
@@ -4684,16 +4638,24 @@ let print_ocaml_binding (name, { args; ret })         pr
"\n";
        pr "static int\n";
        pr "%s_%s_wrapper " name cbname;
-       C.print_arg_list ~user_data:true cbargs;
+       C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
        pr "\n";
        pr "{\n";
-       pr "  int ret;\n";
+       pr "  int ret = 0;\n";
        pr "\n";
+       pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
        pr "  caml_leave_blocking_section ();\n";
        pr "  ret = %s_%s_wrapper_locked " name cbname;
        C.print_arg_list ~user_data:true ~types:false cbargs;
        pr ";\n";
        pr "  caml_enter_blocking_section ();\n";
+       pr "  }\n";
+       pr "\n";
+       pr "  if (valid_flag & LIBNBD_CALLBACK_FREE) {\n";
+       pr "    caml_remove_generational_global_root ((value
*)user_data);\n";
+       pr "    free (user_data);\n";
+       pr "  }\n";
+       pr "\n";
        pr "  return ret;\n";
        pr "}\n";
        pr "\n"
@@ -4768,15 +4730,7 @@ let print_ocaml_binding (name, { args; ret })         pr
"  struct nbd_buffer %s_buf = NBD_buffer_val (%sv);\n" n n;
        pr "  void *%s = %s_buf.data;\n" n n;
        pr "  size_t %s = %s_buf.len;\n" count n
-    | OCamlArg (Closure (false, { cbname })) ->
-       pr "  /* This is safe because we only call this while this
stack\n";
-       pr "   * frame is live.\n";
-       pr "   */\n";
-       pr "  CAMLlocal1 (_%s_user_data);\n" cbname;
-       pr "  value *%s_user_data = &_%s_user_data;\n" cbname
cbname;
-       pr "  _%s_user_data = %sv;\n" cbname cbname;
-       pr "  const void *%s = %s_%s_wrapper;\n" cbname name cbname
-    | OCamlArg (Closure (true, { cbname })) ->
+    | OCamlArg (Closure { cbname }) ->
        pr "  /* The function may save a reference to the closure, so
we\n";
        pr "   * must treat it as a possible GC root.\n";
        pr "   */\n";
@@ -4785,9 +4739,7 @@ let print_ocaml_binding (name, { args; ret })         pr
"  if (%s_user_data == NULL) caml_raise_out_of_memory ();\n" cbname;
        pr "  caml_register_generational_global_root
(%s_user_data);\n" cbname;
        pr "  *%s_user_data = %sv;\n" cbname cbname;
-       pr "  const void *%s = %s_%s_wrapper;\n" cbname name cbname;
-       pr "  nbd_add_close_callback (h, free_%s_%s_user_data,
%s_user_data);\n"
-          name cbname cbname
+       pr "  const void *%s = %s_%s_wrapper;\n" cbname name cbname
     | OCamlArg (Flags _) -> assert false (* see above *)
     | OCamlArg (Int n) ->
        pr "  int %s = Int_val (%sv);\n" n n
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 94875aa..e0fd71d 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -64,9 +64,12 @@
       int error = 0;
 
       assert (cmd->error == 0);
-      if (cmd->cb.fn.read (cmd->cb.fn_user_data, cmd->data,
cmd->count,
+      if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
+                           cmd->cb.fn_user_data,
+                           cmd->data, cmd->count,
                            cmd->offset, LIBNBD_READ_DATA, &error) == -1)
         cmd->error = error ? error : EPROTO;
+      cmd->cb.fn.read = NULL; /* because we've freed it */
     }
 
     SET_NEXT_STATE (%^FINISH_COMMAND);
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index f60232e..2ef8d20 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -298,7 +298,7 @@
          * current error rather than any earlier one. If the callback fails
          * without setting errno, then use the server's error below.
          */
-        if (cmd->cb.fn.read (cmd->cb.fn_user_data,
+        if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
                              cmd->data + (offset - cmd->offset),
                              0, offset, LIBNBD_READ_ERROR, &scratch) == -1)
           if (cmd->error == 0)
@@ -385,7 +385,7 @@
     if (cmd->cb.fn.read) {
       int error = cmd->error;
 
-      if (cmd->cb.fn.read (cmd->cb.fn_user_data,
+      if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
                            cmd->data + (offset - cmd->offset),
                            length - sizeof offset, offset,
                            LIBNBD_READ_DATA, &error) == -1)
@@ -447,7 +447,7 @@
     if (cmd->cb.fn.read) {
       int error = cmd->error;
 
-      if (cmd->cb.fn.read (cmd->cb.fn_user_data,
+      if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
                            cmd->data + offset, length,
                            cmd->offset + offset,
                            LIBNBD_READ_HOLE, &error) == -1)
@@ -499,7 +499,7 @@
       /* Call the caller's extent function. */
       int error = cmd->error;
 
-      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
+      if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
                              meta_context->name, cmd->offset,
                              &h->bs_entries[1], (length-4) / 4,
&error) == -1)
         if (cmd->error == 0)
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 6ea43d5..8f62923 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -170,9 +170,13 @@ save_reply_state (struct nbd_handle *h)
   /* Notify the user */
   if (cmd->cb.callback) {
     int error = cmd->error;
+    int r;
 
     assert (cmd->type != NBD_CMD_DISC);
-    switch (cmd->cb.callback (cmd->cb.user_data, cookie, &error)) {
+    r = cmd->cb.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
+                          cmd->cb.user_data, cookie, &error);
+    cmd->cb.callback = NULL; /* because we've freed it */
+    switch (r) {
     case -1:
       if (error)
         cmd->error = error;
@@ -190,7 +194,7 @@ save_reply_state (struct nbd_handle *h)
     h->cmds_in_flight = cmd->next;
   cmd->next = NULL;
   if (retire)
-    free (cmd);
+    nbd_internal_retire_and_free_command (cmd);
   else {
     if (h->cmds_done_tail != NULL)
       h->cmds_done_tail = h->cmds_done_tail->next = cmd;
diff --git a/generator/states.c b/generator/states.c
index 2d7e197..a7f7ca0 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -123,9 +123,13 @@ void abort_commands (struct nbd_handle *h,
     next = cmd->next;
     if (cmd->cb.callback) {
       int error = cmd->error ? cmd->error : ENOTCONN;
+      int r;
 
       assert (cmd->type != NBD_CMD_DISC);
-      switch (cmd->cb.callback (cmd->cb.user_data, cmd->cookie,
&error)) {
+      r = cmd->cb.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
+                            cmd->cb.user_data, cmd->cookie, &error);
+      cmd->cb.callback = NULL; /* because we've freed it */
+      switch (r) {
       case -1:
         if (error)
           cmd->error = error;
@@ -138,7 +142,7 @@ void abort_commands (struct nbd_handle *h,
     if (cmd->error == 0)
       cmd->error = ENOTCONN;
     if (retire)
-      free (cmd);
+      nbd_internal_retire_and_free_command (cmd);
     else {
       cmd->next = NULL;
       if (h->cmds_done_tail)
diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c
index 8f0087d..1a45c2b 100644
--- a/interop/dirty-bitmap.c
+++ b/interop/dirty-bitmap.c
@@ -42,11 +42,14 @@ struct data {
 };
 
 static int
-cb (void *opaque, const char *metacontext, uint64_t offset,
+cb (unsigned valid_flag, void *opaque, const char *metacontext, uint64_t
offset,
     uint32_t *entries, size_t len, int *error)
 {
   struct data *data = opaque;
 
+  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+    return 0;
+
   /* libnbd does not actually verify that a server is fully compliant
    * to the spec; the asserts marked [qemu-nbd] are thus dependent on
    * the fact that qemu-nbd is compliant.
diff --git a/interop/structured-read.c b/interop/structured-read.c
index 4087c95..4c6e619 100644
--- a/interop/structured-read.c
+++ b/interop/structured-read.c
@@ -48,12 +48,16 @@ struct data {
 };
 
 static int
-read_cb (void *opaque, const void *bufv, size_t count, uint64_t offset,
+read_cb (unsigned valid_flag, void *opaque,
+         const void *bufv, size_t count, uint64_t offset,
          unsigned status, int *error)
 {
   struct data *data = opaque;
   const char *buf = bufv;
 
+  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+    return 0;
+
   /* The NBD spec allows chunks to be reordered; we are relying on the
    * fact that qemu-nbd does not do so.
    */
diff --git a/lib/aio.c b/lib/aio.c
index 5a9841e..34b12ac 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -27,6 +27,24 @@
 
 #include "internal.h"
 
+/* Internal function which retires and frees a command. */
+void
+nbd_internal_retire_and_free_command (struct command *cmd)
+{
+  /* Free the callbacks. */
+  if (cmd->type != NBD_CMD_READ && cmd->cb.fn.extent)
+    cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
+                       NULL, 0, NULL, 0, NULL);
+  if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read)
+    cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
+                     NULL, 0, 0, 0, NULL);
+  if (cmd->cb.callback)
+    cmd->cb.callback (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
+                      0, NULL);
+
+  free (cmd);
+}
+
 int
 nbd_unlocked_aio_get_fd (struct nbd_handle *h)
 {
@@ -91,7 +109,7 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h,
   else
     h->cmds_done = cmd->next;
 
-  free (cmd);
+  nbd_internal_retire_and_free_command (cmd);
 
   /* If the command was successful, return true. */
   if (error == 0)
diff --git a/lib/debug.c b/lib/debug.c
index 12c10f7..7784bd9 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -40,9 +40,11 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
 
 int
 nbd_unlocked_set_debug_callback (struct nbd_handle *h,
-                                 int (*debug_fn) (void *, const char *, const
char *),
-                                 void *data)
+                                 debug_fn debug_fn, void *data)
 {
+  if (h->debug_fn)
+    /* ignore return value */
+    h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
   h->debug_fn = debug_fn;
   h->debug_data = data;
   return 0;
@@ -76,7 +78,7 @@ nbd_internal_debug (struct nbd_handle *h, const char *fs, ...)
 
   if (h->debug_fn)
     /* ignore return value */
-    h->debug_fn (h->debug_data, context, msg);
+    h->debug_fn (LIBNBD_CALLBACK_VALID, h->debug_data, context, msg);
   else
     fprintf (stderr, "libnbd: debug: %s: %s\n", context ? :
"unknown", msg);
  out:
diff --git a/lib/handle.c b/lib/handle.c
index 9c643d8..6f5a4d6 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -35,7 +35,7 @@ free_cmd_list (struct command *list)
 
   for (cmd = list; cmd != NULL; cmd = cmd_next) {
     cmd_next = cmd->next;
-    free (cmd);
+    nbd_internal_retire_and_free_command (cmd);
   }
 }
 
@@ -96,6 +96,10 @@ nbd_close (struct nbd_handle *h)
   if (h == NULL)
     return;
 
+  /* Free user callbacks first. */
+  if (h->debug_fn)
+    h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
+
   for (cc = h->close_callbacks; cc != NULL; cc = cc_next) {
     cc_next = cc->next;
     cc->cb (cc->user_data);
diff --git a/lib/internal.h b/lib/internal.h
index 993075a..75f1b89 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -48,6 +48,7 @@ struct meta_context;
 struct close_callback;
 struct socket;
 struct command;
+typedef int (*debug_fn) (unsigned, void *, const char *, const char *);
 
 struct nbd_handle {
   /* Lock protecting concurrent access to the handle. */
@@ -80,7 +81,7 @@ struct nbd_handle {
 
   /* For debugging. */
   bool debug;
-  int (*debug_fn) (void *, const char *, const char *);
+  debug_fn debug_fn;
   void *debug_data;
 
   /* Linked list of close callbacks. */
@@ -257,12 +258,14 @@ struct socket {
   const struct socket_ops *ops;
 };
 
-typedef int (*extent_fn) (void *user_data,
+typedef int (*extent_fn) (unsigned valid_flag, void *user_data,
                           const char *metacontext, uint64_t offset,
                           uint32_t *entries, size_t nr_entries, int *error);
-typedef int (*read_fn) (void *user_data, const void *buf, size_t count,
+typedef int (*read_fn) (unsigned valid_flag, void *user_data,
+                        const void *buf, size_t count,
                         uint64_t offset, unsigned status, int *error);
-typedef int (*callback_fn) (void *user_data, int64_t cookie, int *error);
+typedef int (*callback_fn) (unsigned valid_flag, void *user_data,
+                            int64_t cookie, int *error);
 
 struct command_cb {
   union {
@@ -288,6 +291,9 @@ struct command {
   uint32_t error; /* Local errno value */
 };
 
+/* aio.c */
+extern void nbd_internal_retire_and_free_command (struct command *);
+
 /* crypto.c */
 extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *,
struct socket *oldsock);
 extern bool nbd_internal_crypto_is_reading (struct nbd_handle *);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 531339d..3b8448e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -69,6 +69,7 @@ check_PROGRAMS = \
 	aio-parallel-load \
 	synch-parallel \
 	meta-base-allocation \
+	closure-lifetimes \
 	$(NULL)
 #	can-cache-flag
 #	can-not-cache-flag
@@ -105,6 +106,7 @@ TESTS = \
 	aio-parallel-load.sh \
 	synch-parallel.sh \
 	meta-base-allocation \
+	closure-lifetimes \
 	$(NULL)
 #	can-cache-flag
 #	can-not-cache-flag
@@ -279,6 +281,11 @@ meta_base_allocation_CPPFLAGS = -I$(top_srcdir)/include
 meta_base_allocation_CFLAGS = $(WARNINGS_CFLAGS)
 meta_base_allocation_LDADD = $(top_builddir)/lib/libnbd.la
 
+closure_lifetimes_SOURCES = closure-lifetimes.c
+closure_lifetimes_CPPFLAGS = -I$(top_srcdir)/include
+closure_lifetimes_CFLAGS = $(WARNINGS_CFLAGS)
+closure_lifetimes_LDADD = $(top_builddir)/lib/libnbd.la
+
 # Testing TLS support.
 if HAVE_GNUTLS
 
diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c
new file mode 100644
index 0000000..a038685
--- /dev/null
+++ b/tests/closure-lifetimes.c
@@ -0,0 +1,136 @@
+/* NBD client library in userspace
+ * Copyright (C) 2013-2019 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
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/* Test closure lifetimes. */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <assert.h>
+
+#include <libnbd.h>
+
+static char *nbdkit[] = { "nbdkit", "-s",
"--exit-with-parent", "-v",
+                          "null", "size=512", NULL };
+
+static unsigned debug_fn_valid;
+static unsigned debug_fn_free;
+static unsigned read_cb_valid;
+static unsigned read_cb_free;
+static unsigned completion_cb_valid;
+static unsigned completion_cb_free;
+
+static int
+debug_fn (unsigned valid_flag, void *opaque,
+          const char *context, const char *msg)
+{
+  if (valid_flag & LIBNBD_CALLBACK_VALID)
+    debug_fn_valid++;
+  if (valid_flag & LIBNBD_CALLBACK_FREE)
+    debug_fn_free++;
+  return 0;
+}
+
+static int
+read_cb (unsigned valid_flag, void *opaque,
+         const void *subbuf, size_t count,
+         uint64_t offset, unsigned status, int *error)
+{
+  if (valid_flag & LIBNBD_CALLBACK_VALID)
+    read_cb_valid++;
+  if (valid_flag & LIBNBD_CALLBACK_FREE)
+    read_cb_free++;
+  return 0;
+}
+
+static int
+completion_cb (unsigned valid_flag, void *opaque,
+               int64_t cookie, int *error)
+{
+  if (valid_flag & LIBNBD_CALLBACK_VALID)
+    completion_cb_valid++;
+  if (valid_flag & LIBNBD_CALLBACK_FREE)
+    completion_cb_free++;
+  return 0;
+}
+
+int
+main (int argc, char *argv[])
+{
+  struct nbd_handle *nbd;
+  int64_t cookie;
+  char buf[512];
+
+  /* Check debug functions are freed when a new debug function is
+   * registered, and when the handle is closed.
+   */
+  nbd = nbd_create ();
+  assert (nbd);
+
+  nbd_set_debug_callback (nbd, debug_fn, NULL);
+  assert (debug_fn_free == 0);
+
+  nbd_set_debug_callback (nbd, debug_fn, NULL);
+  assert (debug_fn_free == 1);
+
+  nbd_close (nbd);
+  assert (debug_fn_free == 2);
+
+  /* Test command callbacks are freed when the command is retired. */
+  nbd = nbd_create ();
+  assert (nbd);
+  assert (nbd_connect_command (nbd, nbdkit) == 0);
+
+  cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0,
+                                              read_cb, NULL,
+                                              completion_cb, NULL, 0);
+  assert (read_cb_free == 0);
+  assert (completion_cb_free == 0);
+  while (!nbd_aio_command_completed (nbd, cookie))
+    assert (nbd_poll (nbd, -1) >= 0);
+
+  assert (read_cb_valid == 1);
+  assert (completion_cb_valid == 1);
+  assert (read_cb_free == 1);
+  assert (completion_cb_free == 1);
+
+  nbd_close (nbd);
+
+  /* Test command callbacks are freed if the handle is closed without
+   * running the commands.
+   *
+   * Note it's possible that nbd_aio_pread_structured_callback might
+   * actually complete the command if the server is very fast.
+   */
+  read_cb_valid = read_cb_free +    completion_cb_valid = completion_cb_free =
0;
+  nbd = nbd_create ();
+  assert (nbd);
+  assert (nbd_connect_command (nbd, nbdkit) == 0);
+
+  cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0,
+                                              read_cb, NULL,
+                                              completion_cb, NULL, 0);
+  nbd_close (nbd);
+
+  assert (read_cb_free == 1);
+  assert (completion_cb_free == 1);
+
+  exit (EXIT_SUCCESS);
+}
diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c
index 95e029b..9e88d6b 100644
--- a/tests/meta-base-allocation.c
+++ b/tests/meta-base-allocation.c
@@ -30,7 +30,8 @@
 
 #include <libnbd.h>
 
-static int check_extent (void *data, const char *metacontext,
+static int check_extent (unsigned valid_flag, void *data,
+                         const char *metacontext,
                          uint64_t offset,
                          uint32_t *entries, size_t nr_entries, int *error);
 
@@ -128,12 +129,18 @@ main (int argc, char *argv[])
 }
 
 static int
-check_extent (void *data, const char *metacontext,
+check_extent (unsigned valid_flag, void *data,
+              const char *metacontext,
               uint64_t offset,
               uint32_t *entries, size_t nr_entries, int *error)
 {
   size_t i;
-  int id = * (int *)data;
+  int id;
+
+  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+    return 0;
+
+  id = * (int *)data;
 
   printf ("extent: id=%d, metacontext=%s, offset=%" PRIu64 ",
"
           "nr_entries=%zu, error=%d\n",
diff --git a/tests/oldstyle.c b/tests/oldstyle.c
index ad22b38..95e1c97 100644
--- a/tests/oldstyle.c
+++ b/tests/oldstyle.c
@@ -37,10 +37,16 @@ static char wbuf[512] = { 1, 2, 3, 4 }, rbuf[512];
 static const char *progname;
 
 static int
-pread_cb (void *data, const void *buf, size_t count, uint64_t offset,
+pread_cb (unsigned valid_flag, void *data,
+          const void *buf, size_t count, uint64_t offset,
           unsigned status, int *error)
 {
-  int *calls = data;
+  int *calls;
+
+  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+    return 0;
+
+  calls = data;
   ++*calls;
 
   if (buf != rbuf || count != sizeof rbuf) {
diff --git a/tests/server-death.c b/tests/server-death.c
index 08e5358..d99854e 100644
--- a/tests/server-death.c
+++ b/tests/server-death.c
@@ -34,8 +34,11 @@ static bool trim_retired;
 static const char *progname;
 
 static int
-callback (void *ignored, int64_t cookie, int *error)
+callback (unsigned valid_flag, void *ignored, int64_t cookie, int *error)
 {
+  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+    return 0;
+
   if (*error != ENOTCONN) {
     fprintf (stderr, "%s: unexpected error in trim callback: %s\n",
              progname, strerror (*error));
-- 
2.22.0

We previously needed nbd_add_close_callback to do cleanup from
language bindings.  However now we have closure lifetimes this is no
longer needed and can be removed.

See also:
https://www.redhat.com/archives/libguestfs/2019-July/msg00213.html
---
 generator/generator | 18 ------------------
 lib/handle.c        | 35 -----------------------------------
 lib/internal.h      | 10 ----------
 3 files changed, 63 deletions(-)

diff --git a/generator/generator b/generator/generator
index 7aad57c..92a1aae 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3055,7 +3055,6 @@ let generate_lib_libnbd_syms ()  
   pr "{\n";
   pr "  global:\n";
-  pr "    nbd_add_close_callback;\n";
   pr "    nbd_create;\n";
   pr "    nbd_close;\n";
   pr "    nbd_get_errno;\n";
@@ -3255,11 +3254,6 @@ let generate_include_libnbd_h ()    pr "extern int
nbd_get_errno (void);\n";
   pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n";
   pr "\n";
-  pr "extern int nbd_add_close_callback (struct nbd_handle *h,\n";
-  pr "                                   nbd_close_callback cb,\n";
-  pr "                                   void *user_data);\n";
-  pr "#define LIBNBD_HAVE_NBD_ADD_CLOSE_CALLBACK 1\n";
-  pr "\n";
   List.iter (
     fun (name, { args; ret }) -> print_extern_and_define name args ret
   ) handle_calls;
@@ -3536,18 +3530,6 @@ errors have corresponding errnos, so even if there has
been an error
 this may return C<0>.  Error codes are the standard ones from
 C<E<lt>errno.hE<gt>>.
 
-=head1 CLOSE CALLBACKS
-
-You can register close callbacks with the handle.  These are
-called when C<nbd_close> is called.  The order in which they
-are called is not defined.  This API is only available
-from C and is designed to help when writing bindings to libnbd
-from other programming languages.
-
- typedef void (*nbd_close_callback) (void *user_data);
- int nbd_add_close_callback (struct nbd_handle *nbd,
-                             nbd_close_callback cb, void *user_data);
-
 ";
 
   pr "=head1 API CALLS\n";
diff --git a/lib/handle.c b/lib/handle.c
index 6f5a4d6..840702a 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -90,7 +90,6 @@ nbd_create (void)
 void
 nbd_close (struct nbd_handle *h)
 {
-  struct close_callback *cc, *cc_next;
   struct meta_context *m, *m_next;
 
   if (h == NULL)
@@ -100,12 +99,6 @@ nbd_close (struct nbd_handle *h)
   if (h->debug_fn)
     h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
 
-  for (cc = h->close_callbacks; cc != NULL; cc = cc_next) {
-    cc_next = cc->next;
-    cc->cb (cc->user_data);
-    free (cc);
-  }
-
   free (h->bs_entries);
   for (m = h->meta_contexts; m != NULL; m = m_next) {
     m_next = m->next;
@@ -202,34 +195,6 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const
char *name)
   return 0;
 }
 
-/* This is not generated because we don't want to offer it to other
- * programming languages.
- */
-int
-nbd_add_close_callback (struct nbd_handle *h,
-                        nbd_close_callback cb, void *user_data)
-{
-  int ret;
-  struct close_callback *cc;
-
-  pthread_mutex_lock (&h->lock);
-  cc = malloc (sizeof *cc);
-  if (cc == NULL) {
-    set_error (errno, "malloc");
-    ret = -1;
-    goto out;
-  }
-  cc->next = h->close_callbacks;
-  cc->cb = cb;
-  cc->user_data = user_data;
-  h->close_callbacks = cc;
-
-  ret = 0;
- out:
-  pthread_mutex_unlock (&h->lock);
-  return ret;
-}
-
 const char *
 nbd_unlocked_get_package_name (struct nbd_handle *h)
 {
diff --git a/lib/internal.h b/lib/internal.h
index 75f1b89..14d2ef0 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -45,7 +45,6 @@
 #define MAX_REQUEST_SIZE (64 * 1024 * 1024)
 
 struct meta_context;
-struct close_callback;
 struct socket;
 struct command;
 typedef int (*debug_fn) (unsigned, void *, const char *, const char *);
@@ -84,9 +83,6 @@ struct nbd_handle {
   debug_fn debug_fn;
   void *debug_data;
 
-  /* Linked list of close callbacks. */
-  struct close_callback *close_callbacks;
-
   /* State machine.
    *
    * The actual current state is ‘state’.  ‘public_state’ is updated
@@ -226,12 +222,6 @@ struct meta_context {
   uint32_t context_id;          /* Context ID negotiated with the server. */
 };
 
-struct close_callback {
-  struct close_callback *next;  /* Linked list. */
-  nbd_close_callback cb;        /* Function. */
-  void *user_data;              /* Data. */
-};
-
 struct socket_ops {
   ssize_t (*recv) (struct nbd_handle *h,
                    struct socket *sock, void *buf, size_t len);
-- 
2.22.0

On 7/25/19 8:07 AM, Richard W.M. Jones wrote:
> Previously closures had a crude flag which tells if they are
> persistent or transient.  Transient closures (flag = false) last for
> the lifetime of the currently called libnbd function.  Persistent
> closures had an indefinite lifetime which could last for as long as
> the handle.  In language bindings handling persistent closures was
> wasteful as we needed to register a "close callback" to free the
> closure when the handle is closed.  But if you had submitted thousands
> of asynchronous commands you would end up registering thousands of
> close callbacks.
> 
> +++ b/.gitignore
> @@ -108,6 +108,7 @@ Makefile.in
>  /tests/can-trim-flag
>  /tests/can-not-trim-flag
>  /tests/can-zero-flag
> +/tests/closure-lifetimes
Always good to see testsuite coverage of new features :)  I guess I'll
see below if I can offer more ideas for what the test can do.
> +
> +The valid flag is only present in the C API.  It is not needed when
> +using garbage-collected programming languages.
> +
> +=head2 Callbacks and locking
>  
>  The callbacks are invoked at a point where the libnbd lock is held; as
>  such, it is unsafe for the callback to call any C<nbd_*> APIs on the
Missing a change to the auto-retire text and error handling text (since
we have places where we now ignore the return value when calling with
only FREE); it may be worth squashing in:

diff --git i/docs/libnbd.pod w/docs/libnbd.pod
index 4d31c64..1fcbfd7 100644
--- i/docs/libnbd.pod
+++ w/docs/libnbd.pod
@@ -501,9 +501,10 @@ complete.  The completion callback will be invoked
with C<cookie> set
 to the same value returned by the original API such as
 C<nbd_aio_pread_callback> (in rare cases, it is possible that the
 completion callback may fire before the original API has returned).
-If the completion callback returns C<1>, the command is automatically
-retired (there is no need to call C<nbd_aio_command_completed>); for
-any other return value, the command still needs to be retired.
+When C<valid_flag> includes C<LIBNBD_CALLBACK_VALID>, and the
+completion callback returns C<1>, the command is automatically retired
+(there is no need to call C<nbd_aio_command_completed>); for any other
+return value, the command still needs to be retired.

 =head2 Callbacks with C<int *error> parameter

@@ -515,8 +516,9 @@ all of the completion callbacks, include a parameter
C<error>
 containing the value of any error detected so far; if the callback
 function fails, it should assign back into C<error> and return
C<-1>
 to change the resulting error of the overall command.  Assignments
-into C<error> are ignored for any other return value; similarly,
+into C<error> are ignored for any other return value or when
+C<valid_flag> did not contain C<LIBNBD_CALLBACK_VALID>; similarly,
 assigning C<0> into C<error> does not have an effect.

 =head1 SEE ALSO

> +++ b/examples/strict-structured-reads.c
>  static int
> -read_verify (void *opaque, int64_t cookie, int *error)
> +read_verify (unsigned valid_flag, void *opaque, int64_t cookie, int
*error)
>  {
> +  int ret = 0;
> +
> +  if (valid_flag & LIBNBD_CALLBACK_VALID) {
>    struct data *data = opaque;
> -  int ret = -1;
>  
> +    ret = -1;
>    total_reads++;
Was this patch posted with whitespace changes ignored?  Otherwise, it
looks like you missed reindenting things.
>    total_chunks += data->chunks;
>    if (*error)
> @@ -160,7 +167,11 @@ read_verify (void *opaque, int64_t cookie, int *error)
>      data->remaining = r->next;
>      free (r);
>    }
> -  free (data);
> +  }
> +
> +  if (valid_flag & LIBNBD_CALLBACK_FREE)
> +    free (opaque);
> +
>    return ret;
>  }
>  
> diff --git a/generator/generator b/generator/generator
> @@ -1350,11 +1346,10 @@ protocol extensions).";
>    "pread_structured", {
>      default_call with
>      args = [ BytesOut ("buf", "count"); UInt64
"offset";
> -             Closure (false,
> -                      { cbname="chunk";
> +             Closure { cbname="chunk";
>                          cbargs=[BytesIn ("subbuf",
"count");
Indentation.
>                                  UInt64 "offset"; UInt
"status";
> -                                Mutable (Int "error")] });
> +                                Mutable (Int "error")] };
>               Flags "flags" ];
>      ret = RErr;
>      permitted_states = [ Connected ];
> @@ -1541,13 +1536,12 @@ punching a hole.";
>    "block_status", {
>      default_call with
>      args = [ UInt64 "count"; UInt64 "offset";
> -             Closure (false,
> -                      { cbname="extent";
> +             Closure { cbname="extent";
>                          cbargs=[String "metacontext";
and again; I'll quit pointing it out for args=[].
> @@ -3714,28 +3707,16 @@ let print_python_binding name { args; ret } 
> +    | Closure { cbname; cbargs } ->
>         pr "/* Wrapper for %s callback of %s. */\n" cbname name;
>         pr "static int\n";
>         pr "%s_%s_wrapper " name cbname;
> -       C.print_arg_list ~user_data:true cbargs;
> +       C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
>         pr "\n";
>         pr "{\n";
> -       pr "  int ret;\n";
> +       pr "  int ret = 0;\n";
> +       pr "\n";
> +       pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
>         pr "  PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
Another place where indentation looks odd, unless this email was
generated with whitespace changes ignored.  (Cleaning up whitespace in a
separate patch to let THIS patch focus on the semantic changes is also
acceptable)

> @@ -4684,16 +4638,24 @@ let print_ocaml_binding (name, { args; ret }) > 
pr "\n";
>         pr "static int\n";
>         pr "%s_%s_wrapper " name cbname;
> -       C.print_arg_list ~user_data:true cbargs;
> +       C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
>         pr "\n";
>         pr "{\n";
> -       pr "  int ret;\n";
> +       pr "  int ret = 0;\n";
>         pr "\n";
> +       pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
>         pr "  caml_leave_blocking_section ();\n";
Same here.

> +++ b/generator/states-reply-structured.c
> @@ -298,7 +298,7 @@
>           * current error rather than any earlier one. If the callback
fails
>           * without setting errno, then use the server's error below.
>           */
> -        if (cmd->cb.fn.read (cmd->cb.fn_user_data,
> +        if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
>                               cmd->data + (offset - cmd->offset),
>                               0, offset, LIBNBD_READ_ERROR, &scratch)
== -1)
>            if (cmd->error == 0)
We could still optimize this file based on NBD_REPLY_FLAG_DONE, but that
can be a followup.
> @@ -499,7 +499,7 @@
>        /* Call the caller's extent function. */
>        int error = cmd->error;
>  
> -      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
> +      if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
>                               meta_context->name, cmd->offset,
>                               &h->bs_entries[1], (length-4) / 4,
&error) == -1)
>          if (cmd->error == 0)
Hmm - no change to the FINISH state, which means you are relying on
command retirement to free chunk/extent instead.  As long as that
happens, we should be okay, though.
> diff --git a/generator/states-reply.c b/generator/states-reply.c
> index 6ea43d5..8f62923 100644
> --- a/generator/states-reply.c
> +++ b/generator/states-reply.c
> @@ -170,9 +170,13 @@ save_reply_state (struct nbd_handle *h)
>    /* Notify the user */
>    if (cmd->cb.callback) {
>      int error = cmd->error;
> +    int r;
>  
>      assert (cmd->type != NBD_CMD_DISC);
> -    switch (cmd->cb.callback (cmd->cb.user_data, cookie,
&error)) {
> +    r = cmd->cb.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
> +                          cmd->cb.user_data, cookie, &error);
> +    cmd->cb.callback = NULL; /* because we've freed it */
> +    switch (r) {
Moving the side effect out of the switch() condition is a reasonable
move; my fault for putting it there in the first place.
>      case -1:
>        if (error)
>          cmd->error = error;
> @@ -190,7 +194,7 @@ save_reply_state (struct nbd_handle *h)
>      h->cmds_in_flight = cmd->next;
>    cmd->next = NULL;
>    if (retire)
> -    free (cmd);
> +    nbd_internal_retire_and_free_command (cmd);
Looks like a nice helper function.  (Side note: using 'git config
diff.orderfile some/file' can be a way to rearrange patches to be easier
to review logically: I think placing lib/internal.h and
generator/generator first, then generator/* and lib/* prior to tests/*
interop/* examples/* would have have made this review easier to read -
maybe I should propose a scripts/git.orderfile similar to nbdkit).

> +++ b/lib/aio.c
> @@ -27,6 +27,24 @@
>  
>  #include "internal.h"
>  
> +/* Internal function which retires and frees a command. */
> +void
> +nbd_internal_retire_and_free_command (struct command *cmd)
> +{
> +  /* Free the callbacks. */
> +  if (cmd->type != NBD_CMD_READ && cmd->cb.fn.extent)
Looks odd that this was not spelled 'cmd->type ==
NBD_CMD_BLOCK_STATUS'.
> +    cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
> +                       NULL, 0, NULL, 0, NULL);
> +  if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read)
> +    cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
> +                     NULL, 0, 0, 0, NULL);
Perhaps we could even have:
switch (cmd->type) {
case NBD_CMD_READ:
  if (cmd->cb.fn.read) ...
  break;
case NBD_CMD_BLOCK_STATUS:
  if (cmd->cb.fn.extent) ...
  break;
default:
  assert (!cmd->cb.fn.read);
}
> +  if (cmd->cb.callback)
> +    cmd->cb.callback (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
> +                      0, NULL);
> +
> +  free (cmd);
> +}
But as written, the function operates correctly. So up to you if you
want to tweak it.

> @@ -96,6 +96,10 @@ nbd_close (struct nbd_handle *h)
>    if (h == NULL)
>      return;
>  
> +  /* Free user callbacks first. */
> +  if (h->debug_fn)
> +    h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
> +
>    for (cc = h->close_callbacks; cc != NULL; cc = cc_next) {
I recommend either setting h->debug_fn = NULL here, or deferring the
FREE callback to after the h->sock->ops->close (h->sock) below.
Otherwise, a future edit to lib/sockets.c to add in a debug statement
there will cause a use-after-free at a distance.
> +++ b/tests/closure-lifetimes.c
> +static int
> +debug_fn (unsigned valid_flag, void *opaque,
> +          const char *context, const char *msg)
> +{
Is it worth assert(!debug_fn_free), to prove that we never have
use-after-free?
> +  if (valid_flag & LIBNBD_CALLBACK_VALID)
> +    debug_fn_valid++;
> +  if (valid_flag & LIBNBD_CALLBACK_FREE)
> +    debug_fn_free++;
> +  return 0;
> +}
> +
> +static int
> +read_cb (unsigned valid_flag, void *opaque,
> +         const void *subbuf, size_t count,
> +         uint64_t offset, unsigned status, int *error)
> +{
Same here.
> +  if (valid_flag & LIBNBD_CALLBACK_VALID)
> +    read_cb_valid++;
> +  if (valid_flag & LIBNBD_CALLBACK_FREE)
> +    read_cb_free++;
> +  return 0;
> +}
> +
> +static int
> +completion_cb (unsigned valid_flag, void *opaque,
> +               int64_t cookie, int *error)
> +{
and again
> +  if (valid_flag & LIBNBD_CALLBACK_VALID)
> +    completion_cb_valid++;
> +  if (valid_flag & LIBNBD_CALLBACK_FREE)
> +    completion_cb_free++;
> +  return 0;
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  struct nbd_handle *nbd;
> +  int64_t cookie;
> +  char buf[512];
> +
> +  /* Check debug functions are freed when a new debug function is
> +   * registered, and when the handle is closed.
> +   */
> +  nbd = nbd_create ();
> +  assert (nbd);
> +
> +  nbd_set_debug_callback (nbd, debug_fn, NULL);
> +  assert (debug_fn_free == 0);
> +
> +  nbd_set_debug_callback (nbd, debug_fn, NULL);
> +  assert (debug_fn_free == 1);
If you add asserts against use-after-free above, you'd also need to
reset debug_fn_free back to 0 here.
> +
> +  nbd_close (nbd);
> +  assert (debug_fn_free == 2);
with knock-on affects to what you assert here.

Is there any way to reliably test whether debug_fn_valid was
incremented? (To some extent, it depends on whether nbd_set_debug was
used, and if we know for sure that we triggered an action that results
in a debug statement).  Testing for >0 is sufficient, testing for a
specific value would be fragile as we may add or remove debug calls in
future refactorings.
> +
> +  /* Test command callbacks are freed when the command is retired. */
> +  nbd = nbd_create ();
> +  assert (nbd);
> +  assert (nbd_connect_command (nbd, nbdkit) == 0);
Side effects in an assert. Nasty. (Maybe you can get away with it if we
explicitly #undef NDEBUG at the top of this file, so the test still
works if someone does ./configure CFLAGS=-DNDEBUG, but splitting the
side effects from the test validation seems wise)
> +
> +  cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0,
> +                                              read_cb, NULL,
> +                                              completion_cb, NULL, 0);
> +  assert (read_cb_free == 0);
> +  assert (completion_cb_free == 0);
> +  while (!nbd_aio_command_completed (nbd, cookie))
> +    assert (nbd_poll (nbd, -1) >= 0);
More side effects in an assert :(
> +
> +  assert (read_cb_valid == 1);
> +  assert (completion_cb_valid == 1);
> +  assert (read_cb_free == 1);
> +  assert (completion_cb_free == 1);
> +
> +  nbd_close (nbd);
> +
> +  /* Test command callbacks are freed if the handle is closed without
> +   * running the commands.
> +   *
> +   * Note it's possible that nbd_aio_pread_structured_callback might
> +   * actually complete the command if the server is very fast.
We can use --filter=delay delay-read=15 to ensure the server is not very
fast, to reduce the risk of the race (but only for this part of the
test, not the earlier part where we do wait for completion - which means
two separate parameter lists for nbd_connect_command() calls...)
> +   */
> +  read_cb_valid = read_cb_free > +    completion_cb_valid =
completion_cb_free = 0;
> +  nbd = nbd_create ();
> +  assert (nbd);
> +  assert (nbd_connect_command (nbd, nbdkit) == 0);
side effects :(
> +
> +  cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0,
> +                                              read_cb, NULL,
> +                                              completion_cb, NULL, 0);
> +  nbd_close (nbd);
> +
> +  assert (read_cb_free == 1);
> +  assert (completion_cb_free == 1);
Worth asserting that read_cb_valid and completion_cb_valid are 0 (if we
are confident in our ability to strand the command unretired due to use
of a filter delay)?
> +
> +  exit (EXIT_SUCCESS);
> +}
> diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c
> index 95e029b..9e88d6b 100644
> --- a/tests/meta-base-allocation.c
Most of my comments can be addressed by followups or pertain to the new
test; I'm okay if you want to push this without posting v4.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 7/25/19 8:07 AM, Richard W.M. Jones wrote:
> We previously needed nbd_add_close_callback to do cleanup from
> language bindings.  However now we have closure lifetimes this is no
> longer needed and can be removed.
> 
> See also:
> https://www.redhat.com/archives/libguestfs/2019-July/msg00213.html
> ---
>  generator/generator | 18 ------------------
>  lib/handle.c        | 35 -----------------------------------
>  lib/internal.h      | 10 ----------
>  3 files changed, 63 deletions(-)
This one is still good to go (but depends on the other, so I see why you
reposted it).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org