Libguestfs - Aug 2019 - Re: [PATCH libnbd 1/7] api: Add semi-private function for freeing persistent data.

As proposed here:
https://www.redhat.com/archives/libguestfs/2019-August/msg00130.html

I didn't actually read Eric's replies to that yet because I've been
concentrating on writing these patches all day.  Anyway here they are
and I'll look at what Eric said about the proposal next.

Rich.

This adds a C-only semi-private function for freeing various types of
persistent data passed to libnbd.

There are some similarities with nbd_add_close_callback which we
removed in commit 7f191b150b52ed50098976309a6af883d245fc56.
---
 generator/generator |  57 ++++++++++++++++++++
 lib/Makefile.am     |   1 +
 lib/free.c          | 129 ++++++++++++++++++++++++++++++++++++++++++++
 lib/handle.c        |   6 +++
 lib/internal.h      |  14 +++++
 5 files changed, 207 insertions(+)

diff --git a/generator/generator b/generator/generator
index c52200b..d6b9352 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3284,6 +3284,7 @@ let generate_lib_libnbd_syms ()        pr
"LIBNBD_%d.%d {\n" major minor;
       pr "  global:\n";
       if (major, minor) = (1, 0) then (
+        pr "    nbd_add_free_callback;\n";
         pr "    nbd_create;\n";
         pr "    nbd_close;\n";
         pr "    nbd_get_errno;\n";
@@ -3581,6 +3582,13 @@ 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 "typedef void (*nbd_free_callback) (void *ptr, void
*user_data);\n";
+  pr "extern int nbd_add_free_callback (struct nbd_handle *h,\n";
+  pr "                                  void *ptr,\n";
+  pr "                                  nbd_free_callback cb,\n";
+  pr "                                  void *user_data);\n";
+  pr "#define LIBNBD_HAVE_NBD_ADD_FREE_CALLBACK 1\n";
+  pr "\n";
   print_closure_typedefs ();
   List.iter (
     fun (name, { args; optargs; ret }) ->
@@ -4005,6 +4013,55 @@ C<E<lt>errno.hE<gt>>.
   List.iter print_api handle_calls;
 
   pr "\
+=head1 FREE CALLBACKS
+
+B<Note:> The API described in this section is only
+available from C.  It is designed to help when writing
+bindings to libnbd in other programming languages.
+As such it is B<not> covered by the usual API stability
+guarantee offered by other parts of the C API.  Use it with care.
+
+Some pointers passed to libnbd calls are saved in the
+S<C<struct nbd_handle>>.  These include pointers to buffers
+passed to C<nbd_aio_pread>, C<nbd_aio_pwrite>, etc.,
+and pointers to the C<user_data> for callbacks.  If you
+want to know when it is safe to free these objects then
+you can register a free callback using:
+
+ typedef void (*nbd_free_callback) (void *ptr, void *user_data);
+ int nbd_add_free_callback (struct nbd_handle *h,
+                            void *ptr,
+                            nbd_free_callback cb,
+                            void *user_data);
+
+C<ptr> is a pointer to the object (ie. the buffer or
+callback C<user_data>).  C<cb (ptr)> is called when libnbd
+no longer holds a reference to that object.
+
+To illustrate how this works with an example:
+
+ struct write_completed_user_data *my_user_data;
+ void *buf;
+ 
+ my_user_data = malloc (sizeof *my_user_data);
+ buf = malloc (512);
+ nbd_add_free_callback (nbd, my_user_data, my_free, NULL);
+ nbd_add_free_callback (nbd, buf, my_free, NULL);
+ nbd_aio_pwrite_callback (nbd, buf, 512, 0,
+                          write_completed, my_user_data, 0);
+
+where C<my_free> is a simple wrapper around the L<free(3)>
+function (we are not using the extra C<user_data> pointer
+in this case):
+
+ void my_free (void *ptr, void *)
+ {
+   free (ptr);
+ }
+
+This would free both C<my_user_data> and C<buf> once libnbd
+has finished with them.
+
 =head1 SEE ALSO
 
 L<libnbd(3)>.
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 3657b9f..08e9d25 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -42,6 +42,7 @@ libnbd_la_SOURCES = \
 	disconnect.c \
 	errors.c \
 	flags.c \
+	free.c \
 	handle.c \
 	internal.h \
 	is-state.c \
diff --git a/lib/free.c b/lib/free.c
new file mode 100644
index 0000000..67ba752
--- /dev/null
+++ b/lib/free.c
@@ -0,0 +1,129 @@
+/* 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
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <assert.h>
+
+#include "internal.h"
+
+/* This is not generated because we don't want to offer it to other
+ * programming languages.  Also it's a semi-private API liable to
+ * evolve in its exact semantics.
+ */
+int
+nbd_add_free_callback (struct nbd_handle *h, void *ptr,
+                       nbd_free_callback cb, void *user_data)
+{
+  int ret = -1;
+  size_t i;
+
+  if (ptr == NULL)
+    return 0;
+
+  nbd_internal_set_error_context ("nbd_add_free_callback");
+  pthread_mutex_lock (&h->lock);
+
+  /* Extend the list of callbacks in the handle. */
+  if (h->nr_free_callbacks >= h->alloc_free_callbacks) {
+    size_t new_alloc;
+    struct free_callback *new_callbacks;
+
+    if (h->alloc_free_callbacks == 0)
+      new_alloc = 8;
+    else
+      new_alloc = 2 * h->alloc_free_callbacks;
+
+    new_callbacks = realloc (h->free_callbacks,
+                             sizeof (struct free_callback) * new_alloc);
+    if (new_callbacks == NULL) {
+      set_error (errno, "realloc");
+      goto out;
+    }
+    h->alloc_free_callbacks = new_alloc;
+    h->free_callbacks = new_callbacks;
+  }
+
+  /* Need to keep the list sorted by pointer so we can use bsearch.
+   * Insert the new entry before the i'th entry.
+   *
+   * Note the same pointer can be added multiple times (eg. if a
+   * buffer is shared between commands), and that is not a bug.  Which
+   * free function is called is undefined, but they should all be
+   * called eventually.
+   */
+  for (i = 0; i < h->nr_free_callbacks; ++i) {
+    if (ptr <= h->free_callbacks[i].ptr)
+      break;
+  }
+  memmove (&h->free_callbacks[i+1], &h->free_callbacks[i],
+           (h->nr_free_callbacks - i) * sizeof (struct free_callback));
+
+  h->free_callbacks[i].ptr = ptr;
+  h->free_callbacks[i].cb = cb;
+  h->free_callbacks[i].user_data = user_data;
+  h->nr_free_callbacks++;
+
+  ret = 0;
+ out:
+  pthread_mutex_unlock (&h->lock);
+  return ret;
+}
+
+static int
+compare_free_callbacks (const void *v1, const void *v2)
+{
+  const void *ptr = v1;
+  const struct free_callback *cb = v2;
+
+  if (ptr < cb->ptr) return -1;
+  else if (ptr > cb->ptr) return 1;
+  else return 0;
+}
+
+/* Called before the library frees 'ptr', where 'ptr' is
something
+ * that might potentially be associated with a free callback.  This is
+ * called often so must be fast.
+ */
+void
+nbd_internal_free_callback (struct nbd_handle *h, void *ptr)
+{
+  struct free_callback *free_cb;
+
+  if (ptr == NULL)
+    return;
+
+  free_cb = bsearch (ptr, h->free_callbacks, h->nr_free_callbacks,
+                     sizeof (struct free_callback),
+                     compare_free_callbacks);
+  if (free_cb) {
+    assert (ptr == free_cb->ptr);
+
+    free_cb->cb (ptr, free_cb->user_data);
+
+    /* Remove it from the free list. */
+    memmove (free_cb, free_cb+1,
+             sizeof (struct free_callback) *
+             (&h->free_callbacks[h->nr_free_callbacks] -
(free_cb+1)));
+    h->nr_free_callbacks--;
+  }
+}
diff --git a/lib/handle.c b/lib/handle.c
index 054c8a7..ae0d196 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -126,6 +126,12 @@ nbd_close (struct nbd_handle *h)
   free_cmd_list (h->cmds_to_issue);
   free_cmd_list (h->cmds_in_flight);
   free_cmd_list (h->cmds_done);
+
+  /* Any remaining free callbacks indicate an error. */
+  if (h->nr_free_callbacks != 0)
+    abort ();
+  free (h->free_callbacks);
+
   nbd_internal_free_string_list (h->argv);
   free (h->unixsocket);
   free (h->hostname);
diff --git a/lib/internal.h b/lib/internal.h
index 301b798..d8b0eed 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -47,6 +47,7 @@
 struct meta_context;
 struct socket;
 struct command;
+struct free_callback;
 
 struct nbd_handle {
   /* Unique name assigned to this handle for debug messages
@@ -87,6 +88,10 @@ struct nbd_handle {
   nbd_debug_callback debug_callback;
   void *debug_data;
 
+  /* Free callbacks kept in pointer order. */
+  struct free_callback *free_callbacks;
+  size_t nr_free_callbacks, alloc_free_callbacks;
+
   /* State machine.
    *
    * The actual current state is ‘state’.  ‘public_state’ is updated
@@ -220,6 +225,12 @@ struct nbd_handle {
   bool disconnect_request;      /* True if we've queued NBD_CMD_DISC */
 };
 
+struct free_callback {
+  void *ptr;
+  nbd_free_callback cb;
+  void *user_data;
+};
+
 struct meta_context {
   struct meta_context *next;    /* Linked list. */
   char *name;                   /* Name of meta context. */
@@ -318,6 +329,9 @@ extern int nbd_internal_set_size_and_flags (struct
nbd_handle *h,
                                             uint64_t exportsize,
                                             uint16_t eflags);
 
+/* free.c */
+extern void nbd_internal_free_callback (struct nbd_handle *h, void *ptr);
+
 /* is-state.c */
 extern bool nbd_internal_is_state_created (enum state state);
 extern bool nbd_internal_is_state_connecting (enum state state);
-- 
2.22.0

When retiring a command test for a free_callback associated with their
buffer.  If there is one call it.  This allows language bindings to
use this mechanism to automatically decrement a reference to the
persistent buffer (note: this patch does not implement this).

The vast majority of this change is simply passing around the handle
so we have it when we call nbd_internal_free_callback in lib/aio.c.
---
 generator/states-reply.c |  2 +-
 generator/states.c       |  2 +-
 lib/aio.c                | 10 ++++++++--
 lib/handle.c             | 10 +++++-----
 lib/internal.h           |  3 ++-
 5 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/generator/states-reply.c b/generator/states-reply.c
index 09adfed..389317e 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -194,7 +194,7 @@ save_reply_state (struct nbd_handle *h)
     h->cmds_in_flight = cmd->next;
   cmd->next = NULL;
   if (retire)
-    nbd_internal_retire_and_free_command (cmd);
+    nbd_internal_retire_and_free_command (h, 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 9ed57ae..a11c1d1 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -142,7 +142,7 @@ void abort_commands (struct nbd_handle *h,
     if (cmd->error == 0)
       cmd->error = ENOTCONN;
     if (retire)
-      nbd_internal_retire_and_free_command (cmd);
+      nbd_internal_retire_and_free_command (h, cmd);
     else {
       cmd->next = NULL;
       if (h->cmds_done_tail)
diff --git a/lib/aio.c b/lib/aio.c
index c141de6..1d26be9 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -29,7 +29,8 @@
 
 /* Internal function which retires and frees a command. */
 void
-nbd_internal_retire_and_free_command (struct command *cmd)
+nbd_internal_retire_and_free_command (struct nbd_handle *h,
+                                      struct command *cmd)
 {
   /* Free the callbacks. */
   if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent)
@@ -42,6 +43,11 @@ nbd_internal_retire_and_free_command (struct command *cmd)
     cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
                         NULL);
 
+  /* Free the persistent buffer if there is one and if there's an
+   * associated free callback.
+   */
+  nbd_internal_free_callback (h, cmd->data);
+
   free (cmd);
 }
 
@@ -109,7 +115,7 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h,
   else
     h->cmds_done = cmd->next;
 
-  nbd_internal_retire_and_free_command (cmd);
+  nbd_internal_retire_and_free_command (h, cmd);
 
   /* If the command was successful, return true. */
   if (error == 0)
diff --git a/lib/handle.c b/lib/handle.c
index ae0d196..0f50e38 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -32,13 +32,13 @@
 #include "internal.h"
 
 static void
-free_cmd_list (struct command *list)
+free_cmd_list (struct nbd_handle *h, struct command *list)
 {
   struct command *cmd, *cmd_next;
 
   for (cmd = list; cmd != NULL; cmd = cmd_next) {
     cmd_next = cmd->next;
-    nbd_internal_retire_and_free_command (cmd);
+    nbd_internal_retire_and_free_command (h, cmd);
   }
 }
 
@@ -123,9 +123,9 @@ nbd_close (struct nbd_handle *h)
     free (m->name);
     free (m);
   }
-  free_cmd_list (h->cmds_to_issue);
-  free_cmd_list (h->cmds_in_flight);
-  free_cmd_list (h->cmds_done);
+  free_cmd_list (h, h->cmds_to_issue);
+  free_cmd_list (h, h->cmds_in_flight);
+  free_cmd_list (h, h->cmds_done);
 
   /* Any remaining free callbacks indicate an error. */
   if (h->nr_free_callbacks != 0)
diff --git a/lib/internal.h b/lib/internal.h
index d8b0eed..42e6921 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -288,7 +288,8 @@ struct command {
 };
 
 /* aio.c */
-extern void nbd_internal_retire_and_free_command (struct command *);
+extern void nbd_internal_retire_and_free_command (struct nbd_handle *,
+                                                  struct command *);
 
 /* crypto.c */
 extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *,
struct socket *oldsock);
-- 
2.22.0

By using the free callback mechanism we don't need to manually free
the buffer.

This reverts part of commit fef1c281a65d061127bf178e5f8cfca0a2475c56.
---
 generator/generator              | 43 +++++++++++++++++++++-----------
 ocaml/buffer.c                   | 22 ----------------
 ocaml/examples/asynch_copy.ml    |  4 +--
 ocaml/libnbd-ocaml.pod           | 22 ----------------
 ocaml/nbd-c.h                    |  8 ++++++
 ocaml/tests/test_590_aio_copy.ml |  4 +--
 6 files changed, 39 insertions(+), 64 deletions(-)

diff --git a/generator/generator b/generator/generator
index d6b9352..92ce170 100755
--- a/generator/generator
+++ b/generator/generator
@@ -4961,10 +4961,6 @@ module Buffer : sig
   (** Allocate an uninitialized buffer.  The parameter is the size
       in bytes. *)
 
-  val free : t -> unit
-  (** The buffer must be manually freed when it is no longer used.
-      An AIO completion callback is usually a good place. *)
-
   val to_bytes : t -> bytes
   (** Copy buffer to an OCaml [bytes] object. *)
 
@@ -5062,7 +5058,6 @@ let ()  module Buffer = struct
   type t
   external alloc : int -> t = \"nbd_internal_ocaml_buffer_alloc\"
-  external free : t -> unit = \"nbd_internal_ocaml_buffer_free\"
   external to_bytes : t -> bytes =
\"nbd_internal_ocaml_buffer_to_bytes\"
   external of_bytes : bytes -> t =
\"nbd_internal_ocaml_buffer_of_bytes\"
   external size : t -> int = \"nbd_internal_ocaml_buffer_size\"
@@ -5244,10 +5239,8 @@ let print_ocaml_binding (name, { args; optargs; ret })   
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 "  if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
+       pr "    free_root (NULL, user_data);\n";
        pr "\n";
        pr "  return ret;\n";
        pr "}\n";
@@ -5313,17 +5306,39 @@ let print_ocaml_binding (name, { args; optargs; ret })  
| BytesIn (n, count) ->
        pr "  const void *%s = Bytes_val (%sv);\n" n n;
        pr "  size_t %s = caml_string_length (%sv);\n" count n
+    | BytesOut (n, count) ->
+       pr "  void *%s = Bytes_val (%sv);\n" n n;
+       pr "  size_t %s = caml_string_length (%sv);\n" count n
     | BytesPersistIn (n, count) ->
+       pr "  /* The function may save a reference to the Buffer, so
we\n";
+       pr "   * must treat it as a possible GC root.\n";
+       pr "   */\n";
+       pr "  value *%s_user_data;\n" n;
+       pr "  %s_user_data = malloc (sizeof (value));\n" n;
+       pr "  if (%s_user_data == NULL) caml_raise_out_of_memory
();\n" n;
+       pr "  *%s_user_data = %sv;\n" n n;
+       pr "  caml_register_generational_global_root
(%s_user_data);\n" n;
        pr "  struct nbd_buffer *%s_buf = NBD_buffer_val (%sv);\n" n
n;
        pr "  const void *%s = %s_buf->data;\n" n n;
-       pr "  size_t %s = %s_buf->len;\n" count n
-    | BytesOut (n, count) ->
-       pr "  void *%s = Bytes_val (%sv);\n" n n;
-       pr "  size_t %s = caml_string_length (%sv);\n" count n
+       pr "  size_t %s = %s_buf->len;\n" count n;
+       pr "  if (nbd_add_free_callback (h, (void *)%s,\n" n;
+       pr "                             free_root, %s_user_data) ==
-1)\n" n;
+       pr "    caml_raise_out_of_memory ();\n"
     | BytesPersistOut (n, count) ->
+       pr "  /* The function may save a reference to the Buffer, so
we\n";
+       pr "   * must treat it as a possible GC root.\n";
+       pr "   */\n";
+       pr "  value *%s_user_data;\n" n;
+       pr "  %s_user_data = malloc (sizeof (value));\n" n;
+       pr "  if (%s_user_data == NULL) caml_raise_out_of_memory
();\n" n;
+       pr "  *%s_user_data = %sv;\n" n n;
+       pr "  caml_register_generational_global_root
(%s_user_data);\n" n;
        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
+       pr "  size_t %s = %s_buf->len;\n" count n;
+       pr "  if (nbd_add_free_callback (h, %s,\n" n;
+       pr "                             free_root, %s_user_data) ==
-1)\n" n;
+       pr "    caml_raise_out_of_memory ();\n"
     | 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";
diff --git a/ocaml/buffer.c b/ocaml/buffer.c
index 837eece..0c0fe00 100644
--- a/ocaml/buffer.c
+++ b/ocaml/buffer.c
@@ -36,10 +36,6 @@ nbd_buffer_finalize (value bv)
 {
   struct nbd_buffer *b = NBD_buffer_val (bv);
 
-  /* Usually if this frees the buffer it indicates a bug in the
-   * program.  The buffer should have been manually freed before
-   * this.  But there's nothing we can do here, so free it.
-   */
   free (b->data);
 }
 
@@ -60,21 +56,6 @@ nbd_internal_ocaml_buffer_alloc (value sizev)
   CAMLreturn (rv);
 }
 
-/* Free an NBD persistent buffer (only the C buffer, not the
- * OCaml object).
- */
-value
-nbd_internal_ocaml_buffer_free (value bv)
-{
-  CAMLparam1 (bv);
-  struct nbd_buffer *b = NBD_buffer_val (bv);
-
-  free (b->data);
-  b->data = NULL;
-
-  CAMLreturn (Val_unit);
-}
-
 /* Copy an NBD persistent buffer to an OCaml bytes. */
 value
 nbd_internal_ocaml_buffer_to_bytes (value bv)
@@ -83,9 +64,6 @@ nbd_internal_ocaml_buffer_to_bytes (value bv)
   CAMLlocal1 (rv);
   struct nbd_buffer *b = NBD_buffer_val (bv);
 
-  if (b->data == NULL)          /* Buffer has been freed. */
-    caml_invalid_argument ("NBD.Buffer.to_bytes used on freed
buffer");
-
   rv = caml_alloc_string (b->len);
   memcpy (Bytes_val (rv), b->data, b->len);
 
diff --git a/ocaml/examples/asynch_copy.ml b/ocaml/examples/asynch_copy.ml
index 7dbe976..50357f9 100644
--- a/ocaml/examples/asynch_copy.ml
+++ b/ocaml/examples/asynch_copy.ml
@@ -31,11 +31,9 @@ let asynch_copy src dst    in
 
   (* This callback is called when any pwrite to the destination
-   * has completed.  We have to manually free the buffer here
-   * as it is no longer used.
+   * has completed.
    *)
   let write_completed buf _ -    NBD.Buffer.free buf;
     (* By returning 1 here we auto-retire the pwrite command. *)
     1
   in
diff --git a/ocaml/libnbd-ocaml.pod b/ocaml/libnbd-ocaml.pod
index f5f968c..5e161c6 100644
--- a/ocaml/libnbd-ocaml.pod
+++ b/ocaml/libnbd-ocaml.pod
@@ -34,28 +34,6 @@ which is the printable error message.  The second is the raw
C<errno>,
 if available (see C<nbd_get_errno>).  The raw C<errno> is not
 compatible with errors in the OCaml C<Unix> module unfortunately.
 
-=head1 AIO BUFFERS
-
-Some libnbd asynchronous I/O (AIO) calls require a buffer parameter
-which persists after the call finishes.  For example C<NBD.aio_pread>
-starts a command which continues reading into the C<NBD.Buffer.t>
-parameter in subsequent calls after the original C<aio_pread> call
-returns.
-
-For these cases you must create a C<NBD.Buffer.t> and ensure that it
-is not garbage collected until the command completes.  The easiest way
-to do this is to use the C<*_callback> variants and free the buffer in
-the callback:
-
- let buf = NBD.Buffer.alloc 512 in
- NBD.aio_pread_callback h buf 0_L (
-   (* This is called when the command has completed. *)
-   fun _ ->
-     NBD.Buffer.free buf;
-     (* Returning 1 from this callback auto-retires the command. *)
-     1
- )
-
 =head1 EXAMPLES
 
 This directory contains examples written in OCaml:
diff --git a/ocaml/nbd-c.h b/ocaml/nbd-c.h
index ffd51d2..a84d64e 100644
--- a/ocaml/nbd-c.h
+++ b/ocaml/nbd-c.h
@@ -107,4 +107,12 @@ struct callback_data {
 extern char **nbd_internal_ocaml_string_list (value);
 extern value nbd_internal_ocaml_alloc_int32_array (uint32_t *, size_t);
 
+/* Free a generational global root and its container. */
+static inline void
+free_root (void *ptr /* unused */, void *rootp)
+{
+  caml_remove_generational_global_root ((value *)rootp);
+  free (rootp);
+}
+
 #endif /* LIBNBD_NBD_C_H */
diff --git a/ocaml/tests/test_590_aio_copy.ml b/ocaml/tests/test_590_aio_copy.ml
index 290ca93..effc299 100644
--- a/ocaml/tests/test_590_aio_copy.ml
+++ b/ocaml/tests/test_590_aio_copy.ml
@@ -53,11 +53,9 @@ let asynch_copy src dst    in
 
   (* This callback is called when any pwrite to the destination
-   * has completed.  We have to manually free the buffer here
-   * as it is no longer used.
+   * has completed.
    *)
   let write_completed buf _ -    NBD.Buffer.free buf;
     bytes_written := !bytes_written + NBD.Buffer.size buf;
     (* By returning 1 here we auto-retire the pwrite command. *)
     1
-- 
2.22.0

Mechanical change: Wherever we call any closure with the
LIBNBD_CALLBACK_FREE function, we also call nbd_internal_free_callback
with the closure's user_data.  This allows calls to associate a free
callback with any closure via its user_data pointer.
---
 generator/states-reply-simple.c     |  1 +
 generator/states-reply-structured.c | 24 ++++++++++++++++++------
 generator/states-reply.c            |  1 +
 generator/states.c                  |  1 +
 lib/aio.c                           |  4 ++++
 lib/debug.c                         |  4 +++-
 lib/handle.c                        |  4 +++-
 7 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 9b249ab..9684bc4 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -69,6 +69,7 @@
                             cmd->data, cmd->count,
                             cmd->offset, LIBNBD_READ_DATA, &error) ==
-1)
         cmd->error = error ? error : EPROTO;
+      nbd_internal_free_callback (h, cmd->cb.fn_user_data);
       cmd->cb.fn.chunk = NULL; /* because we've freed it */
     }
 
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index cdd9f10..b016cd7 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -317,8 +317,10 @@ valid_flags (struct nbd_handle *h)
                               0, offset, LIBNBD_READ_ERROR, &scratch) ==
-1)
           if (cmd->error == 0)
             cmd->error = scratch;
-        if (valid & LIBNBD_CALLBACK_FREE)
+        if (valid & LIBNBD_CALLBACK_FREE) {
           cmd->cb.fn.chunk = NULL; /* because we've freed it */
+          nbd_internal_free_callback (h, cmd->cb.fn_user_data);
+        }
       }
     }
 
@@ -408,8 +410,10 @@ valid_flags (struct nbd_handle *h)
                            LIBNBD_READ_DATA, &error) == -1)
         if (cmd->error == 0)
           cmd->error = error ? error : EPROTO;
-      if (valid & LIBNBD_CALLBACK_FREE)
+      if (valid & LIBNBD_CALLBACK_FREE) {
         cmd->cb.fn.chunk = NULL; /* because we've freed it */
+        nbd_internal_free_callback (h, cmd->cb.fn_user_data);
+      }
     }
 
     SET_NEXT_STATE (%FINISH);
@@ -473,8 +477,10 @@ valid_flags (struct nbd_handle *h)
                             LIBNBD_READ_HOLE, &error) == -1)
         if (cmd->error == 0)
           cmd->error = error ? error : EPROTO;
-      if (valid & LIBNBD_CALLBACK_FREE)
+      if (valid & LIBNBD_CALLBACK_FREE) {
         cmd->cb.fn.chunk = NULL; /* because we've freed it */
+        nbd_internal_free_callback (h, cmd->cb.fn_user_data);
+      }
     }
 
     SET_NEXT_STATE(%FINISH);
@@ -527,8 +533,10 @@ valid_flags (struct nbd_handle *h)
                              &h->bs_entries[1], (length-4) / 4,
&error) == -1)
         if (cmd->error == 0)
           cmd->error = error ? error : EPROTO;
-      if (valid & LIBNBD_CALLBACK_FREE)
+      if (valid & LIBNBD_CALLBACK_FREE) {
         cmd->cb.fn.extent = NULL; /* because we've freed it */
+        nbd_internal_free_callback (h, cmd->cb.fn_user_data);
+      }
     }
     else
       /* Emit a debug message, but ignore it. */
@@ -545,12 +553,16 @@ valid_flags (struct nbd_handle *h)
 
   flags = be16toh (h->sbuf.sr.structured_reply.flags);
   if (flags & NBD_REPLY_FLAG_DONE) {
-    if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent)
+    if (cmd->type == NBD_CMD_BLOCK_STATUS && 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.chunk)
+      nbd_internal_free_callback (h, cmd->cb.fn_user_data);
+    }
+    if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) {
       cmd->cb.fn.chunk (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
                         NULL, 0, 0, 0, NULL);
+      nbd_internal_free_callback (h, cmd->cb.fn_user_data);
+    }
     cmd->cb.fn.chunk = NULL;
     SET_NEXT_STATE (%^FINISH_COMMAND);
   }
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 389317e..d5cba1a 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -175,6 +175,7 @@ save_reply_state (struct nbd_handle *h)
     assert (cmd->type != NBD_CMD_DISC);
     r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
                             cmd->cb.user_data, &error);
+    nbd_internal_free_callback (h, cmd->cb.user_data);
     cmd->cb.completion = NULL; /* because we've freed it */
     switch (r) {
     case -1:
diff --git a/generator/states.c b/generator/states.c
index a11c1d1..313d2c9 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -128,6 +128,7 @@ void abort_commands (struct nbd_handle *h,
       assert (cmd->type != NBD_CMD_DISC);
       r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
                               cmd->cb.user_data, &error);
+      nbd_internal_free_callback (h, cmd->cb.user_data);
       cmd->cb.completion = NULL; /* because we've freed it */
       switch (r) {
       case -1:
diff --git a/lib/aio.c b/lib/aio.c
index 1d26be9..8aa4547 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -43,6 +43,10 @@ nbd_internal_retire_and_free_command (struct nbd_handle *h,
     cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
                         NULL);
 
+  /* Free the closures if there's an associated free callback. */
+  nbd_internal_free_callback (h, cmd->cb.fn_user_data);
+  nbd_internal_free_callback (h, cmd->cb.user_data);
+
   /* Free the persistent buffer if there is one and if there's an
    * associated free callback.
    */
diff --git a/lib/debug.c b/lib/debug.c
index ad4d9cb..34d4184 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -42,9 +42,11 @@ int
 nbd_unlocked_set_debug_callback (struct nbd_handle *h,
                                  nbd_debug_callback debug_callback, void *data)
 {
-  if (h->debug_callback)
+  if (h->debug_callback) {
     /* ignore return value */
     h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
+    nbd_internal_free_callback (h, h->debug_data);
+  }
   h->debug_callback = debug_callback;
   h->debug_data = data;
   return 0;
diff --git a/lib/handle.c b/lib/handle.c
index 0f50e38..5a47bd8 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -113,8 +113,10 @@ nbd_close (struct nbd_handle *h)
     return;
 
   /* Free user callbacks first. */
-  if (h->debug_callback)
+  if (h->debug_callback) {
     h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
+    nbd_internal_free_callback (h, h->debug_data);
+  }
   h->debug_callback = NULL;
 
   free (h->bs_entries);
-- 
2.22.0

Instead of using the valid_flag == FREE mechanism, use a free callback
to free each closure root.
---
 generator/generator | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/generator/generator b/generator/generator
index 92ce170..109fad6 100755
--- a/generator/generator
+++ b/generator/generator
@@ -5239,9 +5239,6 @@ let print_ocaml_binding (name, { args; optargs; ret })    
pr "  caml_enter_blocking_section ();\n";
        pr "  }\n";
        pr "\n";
-       pr "  if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
-       pr "    free_root (NULL, user_data);\n";
-       pr "\n";
        pr "  return ret;\n";
        pr "}\n";
        pr "\n"
@@ -5348,7 +5345,11 @@ let print_ocaml_binding (name, { args; optargs; ret })   
pr "  if (%s_user_data == NULL) caml_raise_out_of_memory ();\n"
cbname;
        pr "  *%s_user_data = %sv;\n" cbname cbname;
        pr "  caml_register_generational_global_root
(%s_user_data);\n" cbname;
-       pr "  const void *%s_callback = %s_%s_wrapper;\n" cbname name
cbname
+       pr "  const void *%s_callback = %s_%s_wrapper;\n" cbname name
cbname;
+       pr "  if (nbd_add_free_callback (h, %s_user_data,\n" cbname;
+       pr "                             free_root, %s_user_data) ==
-1)\n"
+         cbname;
+       pr "    caml_raise_out_of_memory ();\n"
     | Enum (n, { enum_prefix }) ->
        pr "  int %s = %s_val (%sv);\n" n enum_prefix n
     | Flags (n, { flag_prefix }) ->
-- 
2.22.0

Same as the previous commit, but for Python.
---
 generator/generator | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/generator/generator b/generator/generator
index 109fad6..a0322ee 100755
--- a/generator/generator
+++ b/generator/generator
@@ -4315,9 +4315,6 @@ let print_python_binding name { args; optargs; ret;
may_set_error }         ) 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"
@@ -4454,6 +4451,11 @@ let print_python_binding name { args; optargs; ret;
may_set_error }         pr "    PyErr_SetString (PyExc_TypeError,\n";
        pr "                     \"callback parameter %s is not
callable\");\n" cbname;
        pr "    return NULL;\n";
+       pr "  }\n";
+       pr "  if (nbd_add_free_callback (h, %s_user_data,\n" cbname;
+       pr "                             decref, NULL) == -1) {\n";
+       pr "    PyErr_NoMemory ();\n";
+       pr "    return NULL;\n";
        pr "  }\n"
     | Enum _ -> ()
     | Flags (n, _) -> pr "  %s_u32 = %s;\n" n n
@@ -4600,6 +4602,12 @@ let generate_python_methods_c ()    pr "\n";
   pr "#include <methods.h>\n";
   pr "\n";
+  pr "static void\n";
+  pr "decref (void *ptr, void *user_data)\n";
+  pr "{\n";
+  pr "  Py_DECREF (ptr);\n";
+  pr "}\n";
+  pr "\n";
   List.iter (
     fun (name, fn) ->
       print_python_binding name fn
-- 
2.22.0

The freeing feature can now be done by associating a free callback
with a closure's user_data, so having the valid_flag is no longer
necessary and it can be completely removed.

This mostly reverts commit 2d9b98e96772e282d51dafac07f16387dadc8afa.
---
 TODO                                |   2 -
 docs/libnbd.pod                     |  64 ++--------------
 examples/glib-main-loop.c           |  14 +---
 examples/strict-structured-reads.c  |  63 +++++++---------
 generator/generator                 |  84 +++++++++------------
 generator/states-reply-simple.c     |   3 +-
 generator/states-reply-structured.c |  45 ++++--------
 generator/states-reply.c            |   3 +-
 generator/states.c                  |   3 +-
 interop/dirty-bitmap.c              |   5 +-
 interop/structured-read.c           |   5 +-
 lib/aio.c                           |  11 ---
 lib/debug.c                         |   7 +-
 lib/handle.c                        |   4 +-
 tests/closure-lifetimes.c           | 109 +++++++++++++++++-----------
 tests/meta-base-allocation.c        |   7 +-
 tests/oldstyle.c                    |   5 +-
 tests/server-death.c                |   5 +-
 18 files changed, 165 insertions(+), 274 deletions(-)

diff --git a/TODO b/TODO
index 8e067c0..03fda1f 100644
--- a/TODO
+++ b/TODO
@@ -26,8 +26,6 @@ Improve function trace output so that:
 Suggested API improvements:
   general:
   - synchronous APIs that have a timeout or can be cancelled
-  - separate free_callback corresponding to every Closure
-    (instead of using valid_flag)
 
   connecting:
   - nbd_connect_tcp: allow control over whether IPv4 or IPv6 is desired
diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index 51b1a03..0068b84 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -608,59 +608,6 @@ 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
@@ -673,10 +620,10 @@ All of the low-level commands have a completion callback
variant that
 registers a callback function used right before the command is marked
 complete.
 
-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.
+When 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
 
@@ -688,8 +635,7 @@ 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 or when
-C<valid_flag> did not contain C<LIBNBD_CALLBACK_VALID>; similarly,
+into C<error> are ignored for any other return value; similarly,
 assigning C<0> into C<error> does not have an effect.
 
 =head1 SEE ALSO
diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c
index 05a59e3..216eb5e 100644
--- a/examples/glib-main-loop.c
+++ b/examples/glib-main-loop.c
@@ -268,9 +268,9 @@ static GMainLoop *loop;
 
 static void connected (struct NBDSource *source);
 static gboolean read_data (gpointer user_data);
-static int finished_read (unsigned valid_flag, void *vp, int *error);
+static int finished_read (void *vp, int *error);
 static gboolean write_data (gpointer user_data);
-static int finished_write (unsigned valid_flag, void *vp, int *error);
+static int finished_write (void *vp, int *error);
 
 int
 main (int argc, char *argv[])
@@ -394,13 +394,10 @@ read_data (gpointer user_data)
 
 /* This callback is called from libnbd when any read command finishes. */
 static int
-finished_read (unsigned valid_flag, void *vp, int *error)
+finished_read (void *vp, int *error)
 {
   struct buffer *buffer = vp;
 
-  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
-    return 0;
-
   if (gssrc == NULL)
     return 0;
 
@@ -442,13 +439,10 @@ write_data (gpointer user_data)
 
 /* This callback is called from libnbd when any write command finishes. */
 static int
-finished_write (unsigned valid_flag, void *vp, int *error)
+finished_write (void *vp, int *error)
 {
   struct buffer *buffer = vp;
 
-  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 b3880b7..701b216 100644
--- a/examples/strict-structured-reads.c
+++ b/examples/strict-structured-reads.c
@@ -51,16 +51,13 @@ static int64_t total_bytes;
 static int total_success;
 
 static int
-read_chunk (unsigned valid_flag, void *opaque,
+read_chunk (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);
@@ -131,46 +128,41 @@ read_chunk (unsigned valid_flag, void *opaque,
 }
 
 static int
-read_verify (unsigned valid_flag, void *opaque, int *error)
+read_verify (void *opaque, int *error)
 {
   int ret = 0;
 
-  if (valid_flag & LIBNBD_CALLBACK_VALID) {
-    struct data *data = opaque;
+  struct data *data = opaque;
 
-    ret = -1;
-    total_reads++;
-    total_chunks += data->chunks;
-    if (*error)
-      goto cleanup;
-    assert (data->chunks > 0);
-    if (data->flags & LIBNBD_CMD_FLAG_DF) {
-      total_df_reads++;
-      if (data->chunks > 1) {
-        fprintf (stderr, "buggy server: too many chunks for DF
flag\n");
-        *error = EPROTO;
-        goto cleanup;
-      }
-    }
-    if (data->remaining && !*error) {
-      fprintf (stderr, "buggy server: not enough chunks on
success\n");
+  ret = -1;
+  total_reads++;
+  total_chunks += data->chunks;
+  if (*error)
+    goto cleanup;
+  assert (data->chunks > 0);
+  if (data->flags & LIBNBD_CMD_FLAG_DF) {
+    total_df_reads++;
+    if (data->chunks > 1) {
+      fprintf (stderr, "buggy server: too many chunks for DF
flag\n");
       *error = EPROTO;
       goto cleanup;
     }
-    total_bytes += data->count;
-    total_success++;
-    ret = 0;
-
-  cleanup:
-    while (data->remaining) {
-      struct range *r = data->remaining;
-      data->remaining = r->next;
-      free (r);
-    }
   }
+  if (data->remaining && !*error) {
+    fprintf (stderr, "buggy server: not enough chunks on success\n");
+    *error = EPROTO;
+    goto cleanup;
+  }
+  total_bytes += data->count;
+  total_success++;
+  ret = 0;
 
-  if (valid_flag & LIBNBD_CALLBACK_FREE)
-    free (opaque);
+ cleanup:
+  while (data->remaining) {
+    struct range *r = data->remaining;
+    data->remaining = r->next;
+    free (r);
+  }
 
   return ret;
 }
@@ -235,6 +227,7 @@ main (int argc, char *argv[])
     *r = (struct range) { .first = offset, .last = offset + maxsize, };
     *d = (struct data) { .offset = offset, .count = maxsize, .flags = flags,
                          .remaining = r, };
+    /* XXX *d and *r are both leaked. */
     if (nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, offset,
                                            read_chunk, d, read_verify, d,
                                            flags) == -1) {
diff --git a/generator/generator b/generator/generator
index a0322ee..0187c02 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3190,7 +3190,7 @@ module C : sig
   val generate_docs_libnbd_api_pod : unit -> unit
   val print_arg_list : ?handle:bool -> ?types:bool ->
                        arg list -> optarg list -> unit
-  val print_cbarg_list : ?valid_flag:bool -> ?types:bool -> cbarg list
-> unit
+  val print_cbarg_list : ?types:bool -> cbarg list -> unit
   val errcode_of_ret : ret -> string option
   val type_of_ret : ret -> string
 end = struct
@@ -3430,13 +3430,8 @@ let print_extern name args optargs ret    print_call name
args optargs ret;
   pr ";\n"
 
-let print_cbarg_list ?(valid_flag = true) ?(types = true) cbargs +let
print_cbarg_list ?(types = true) cbargs    pr "(";
-  if valid_flag then (
-    if types then pr "unsigned ";
-    pr "valid_flag";
-    pr ", ";
-  );
   if types then pr "void *";
   pr "user_data";
 
@@ -3567,9 +3562,6 @@ let generate_include_libnbd_h ()        pr "#define
%-40s %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";
@@ -4226,26 +4218,25 @@ let print_python_binding name { args; optargs; ret;
may_set_error }         pr "{\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";
+       pr "  PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
+       pr "  PyObject *py_args, *py_ret;\n";
        List.iter (
          function
          | CBArrayAndLen (UInt32 n, len) ->
-            pr "    PyObject *py_%s = PyList_New (%s);\n" n len;
-            pr "    for (size_t i = 0; i < %s; ++i)\n" len;
-            pr "      PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong
(%s[i]));\n" n n
+            pr "  PyObject *py_%s = PyList_New (%s);\n" n len;
+            pr "  for (size_t i = 0; i < %s; ++i)\n" len;
+            pr "    PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong
(%s[i]));\n" n n
          | CBBytesIn _
          | CBInt _
          | CBInt64 _ -> ()
          | CBMutable (Int n) ->
-            pr "    PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
-            pr "    if (!py_%s_modname) { PyErr_PrintEx (0); return -1;
}\n" n;
-            pr "    PyObject *py_%s_mod = PyImport_Import
(py_%s_modname);\n" n n;
-            pr "    Py_DECREF (py_%s_modname);\n" n;
-            pr "    if (!py_%s_mod) { PyErr_PrintEx (0); return -1;
}\n" n;
-            pr "    PyObject *py_%s = PyObject_CallMethod (py_%s_mod,
\"c_int\", \"i\", *%s);\n" n n n;
-            pr "    if (!py_%s) { PyErr_PrintEx (0); return -1; }\n"
n;
+            pr "  PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
+            pr "  if (!py_%s_modname) { PyErr_PrintEx (0); return -1;
}\n" n;
+            pr "  PyObject *py_%s_mod = PyImport_Import
(py_%s_modname);\n" n n;
+            pr "  Py_DECREF (py_%s_modname);\n" n;
+            pr "  if (!py_%s_mod) { PyErr_PrintEx (0); return -1;
}\n" n;
+            pr "  PyObject *py_%s = PyObject_CallMethod (py_%s_mod,
\"c_int\", \"i\", *%s);\n" n n n;
+            pr "  if (!py_%s) { PyErr_PrintEx (0); return -1; }\n" n;
          | CBString _
          | CBUInt _
          | CBUInt64 _ -> ()
@@ -4253,7 +4244,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }         ) cbargs;
        pr "\n";
 
-       pr "    py_args = Py_BuildValue (\"(\"";
+       pr "  py_args = Py_BuildValue (\"(\"";
        List.iter (
          function
          | CBArrayAndLen (UInt32 n, len) -> pr " \"O\""
@@ -4278,42 +4269,41 @@ let print_python_binding name { args; optargs; ret;
may_set_error }           | CBArrayAndLen _ | CBMutable _ -> assert false
        ) cbargs;
        pr ");\n";
-       pr "    Py_INCREF (py_args);\n";
+       pr "  Py_INCREF (py_args);\n";
        pr "\n";
-       pr "    if (PyEval_ThreadsInitialized ())\n";
-       pr "      py_save = PyGILState_Ensure ();\n";
+       pr "  if (PyEval_ThreadsInitialized ())\n";
+       pr "    py_save = PyGILState_Ensure ();\n";
        pr "\n";
-       pr "    py_ret = PyObject_CallObject ((PyObject *)user_data,
py_args);\n";
+       pr "  py_ret = PyObject_CallObject ((PyObject *)user_data,
py_args);\n";
        pr "\n";
-       pr "    if (PyEval_ThreadsInitialized ())\n";
-       pr "      PyGILState_Release (py_save);\n";
+       pr "  if (PyEval_ThreadsInitialized ())\n";
+       pr "    PyGILState_Release (py_save);\n";
        pr "\n";
-       pr "    Py_DECREF (py_args);\n";
+       pr "  Py_DECREF (py_args);\n";
        pr "\n";
-       pr "    if (py_ret != NULL) {\n";
-       pr "      Py_DECREF (py_ret); /* return value is discarded
*/\n";
-       pr "    }\n";
-       pr "    else {\n";
-       pr "      ret = -1;\n";
-       pr "      PyErr_PrintEx (0); /* print exception */\n";
-       pr "    };\n";
+       pr "  if (py_ret != NULL) {\n";
+       pr "    Py_DECREF (py_ret); /* return value is discarded
*/\n";
+       pr "  }\n";
+       pr "  else {\n";
+       pr "    ret = -1;\n";
+       pr "    PyErr_PrintEx (0); /* print exception */\n";
+       pr "  };\n";
        pr "\n";
        List.iter (
          function
          | CBArrayAndLen (UInt32 n, _) ->
-            pr "    Py_DECREF (py_%s);\n" n
+            pr "  Py_DECREF (py_%s);\n" n
          | CBMutable (Int n) ->
-            pr "    PyObject *py_%s_ret = PyObject_GetAttrString (py_%s,
\"value\");\n" n n;
-            pr "    *%s = PyLong_AsLong (py_%s_ret);\n" n n;
-            pr "    Py_DECREF (py_%s_ret);\n" n;
-            pr "    Py_DECREF (py_%s);\n" n
+            pr "  PyObject *py_%s_ret = PyObject_GetAttrString (py_%s,
\"value\");\n" n n;
+            pr "  *%s = PyLong_AsLong (py_%s_ret);\n" n n;
+            pr "  Py_DECREF (py_%s_ret);\n" n;
+            pr "  Py_DECREF (py_%s);\n" n
          | CBBytesIn _
          | CBInt _ | CBInt64 _
          | CBString _
          | CBUInt _ | CBUInt64 _ -> ()
          | CBArrayAndLen _ | CBMutable _ -> assert false
        ) cbargs;
-       pr "  }\n";
        pr "\n";
        pr "  return ret;\n";
        pr "}\n";
@@ -5159,7 +5149,7 @@ let print_ocaml_binding (name, { args; optargs; ret })    
pr "/* Wrapper for %s callback of %s. */\n" cbname name;
        pr "static int\n";
        pr "%s_%s_wrapper_locked " name cbname;
-       C.print_cbarg_list ~valid_flag:false cbargs;
+       C.print_cbarg_list cbargs;
        pr "\n";
        pr "{\n";
        pr "  CAMLparam0 ();\n";
@@ -5239,13 +5229,11 @@ let print_ocaml_binding (name, { args; optargs; ret })  
pr "{\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_cbarg_list ~valid_flag:false ~types:false cbargs;
+       C.print_cbarg_list ~types:false cbargs;
        pr ";\n";
        pr "  caml_enter_blocking_section ();\n";
-       pr "  }\n";
        pr "\n";
        pr "  return ret;\n";
        pr "}\n";
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 9684bc4..11354e5 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -64,8 +64,7 @@
       int error = 0;
 
       assert (cmd->error == 0);
-      if (cmd->cb.fn.chunk (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
-                            cmd->cb.fn_user_data,
+      if (cmd->cb.fn.chunk (cmd->cb.fn_user_data,
                             cmd->data, cmd->count,
                             cmd->offset, LIBNBD_READ_DATA, &error) ==
-1)
         cmd->error = error ? error : EPROTO;
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index b016cd7..846cc35 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -18,17 +18,6 @@
 
 /* State machine for parsing structured replies from the server. */
 
-static unsigned
-valid_flags (struct nbd_handle *h)
-{
-  unsigned valid = LIBNBD_CALLBACK_VALID;
-  uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
-
-  if (flags & NBD_REPLY_FLAG_DONE)
-    valid |= LIBNBD_CALLBACK_FREE;
-  return valid;
-}
-
 /*----- End of prologue. -----*/
 
 /* STATE MACHINE */ {
@@ -306,18 +295,18 @@ valid_flags (struct nbd_handle *h)
       }
       if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) {
         int scratch = error;
-        unsigned valid = valid_flags (h);
+        uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
 
         /* Different from successful reads: inform the callback about the
          * 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.chunk (valid, cmd->cb.fn_user_data,
+        if (cmd->cb.fn.chunk (cmd->cb.fn_user_data,
                               cmd->data + (offset - cmd->offset),
                               0, offset, LIBNBD_READ_ERROR, &scratch) ==
-1)
           if (cmd->error == 0)
             cmd->error = scratch;
-        if (valid & LIBNBD_CALLBACK_FREE) {
+        if (flags & NBD_REPLY_FLAG_DONE) {
           cmd->cb.fn.chunk = NULL; /* because we've freed it */
           nbd_internal_free_callback (h, cmd->cb.fn_user_data);
         }
@@ -402,15 +391,15 @@ valid_flags (struct nbd_handle *h)
     assert (cmd); /* guaranteed by CHECK */
     if (cmd->cb.fn.chunk) {
       int error = cmd->error;
-      unsigned valid = valid_flags (h);
+      uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
 
-      if (cmd->cb.fn.chunk (valid, cmd->cb.fn_user_data,
+      if (cmd->cb.fn.chunk (cmd->cb.fn_user_data,
                             cmd->data + (offset - cmd->offset),
                             length - sizeof offset, offset,
                            LIBNBD_READ_DATA, &error) == -1)
         if (cmd->error == 0)
           cmd->error = error ? error : EPROTO;
-      if (valid & LIBNBD_CALLBACK_FREE) {
+      if (flags & NBD_REPLY_FLAG_DONE) {
         cmd->cb.fn.chunk = NULL; /* because we've freed it */
         nbd_internal_free_callback (h, cmd->cb.fn_user_data);
       }
@@ -469,15 +458,15 @@ valid_flags (struct nbd_handle *h)
     memset (cmd->data + offset, 0, length);
     if (cmd->cb.fn.chunk) {
       int error = cmd->error;
-      unsigned valid = valid_flags (h);
+      uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
 
-      if (cmd->cb.fn.chunk (valid, cmd->cb.fn_user_data,
+      if (cmd->cb.fn.chunk (cmd->cb.fn_user_data,
                             cmd->data + offset, length,
                             cmd->offset + offset,
                             LIBNBD_READ_HOLE, &error) == -1)
         if (cmd->error == 0)
           cmd->error = error ? error : EPROTO;
-      if (valid & LIBNBD_CALLBACK_FREE) {
+      if (flags & NBD_REPLY_FLAG_DONE) {
         cmd->cb.fn.chunk = NULL; /* because we've freed it */
         nbd_internal_free_callback (h, cmd->cb.fn_user_data);
       }
@@ -526,14 +515,14 @@ valid_flags (struct nbd_handle *h)
     if (meta_context) {
       /* Call the caller's extent function. */
       int error = cmd->error;
-      unsigned valid = valid_flags (h);
+      uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
 
-      if (cmd->cb.fn.extent (valid, cmd->cb.fn_user_data,
+      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
                              meta_context->name, cmd->offset,
                              &h->bs_entries[1], (length-4) / 4,
&error) == -1)
         if (cmd->error == 0)
           cmd->error = error ? error : EPROTO;
-      if (valid & LIBNBD_CALLBACK_FREE) {
+      if (flags & NBD_REPLY_FLAG_DONE) {
         cmd->cb.fn.extent = NULL; /* because we've freed it */
         nbd_internal_free_callback (h, cmd->cb.fn_user_data);
       }
@@ -553,16 +542,10 @@ valid_flags (struct nbd_handle *h)
 
   flags = be16toh (h->sbuf.sr.structured_reply.flags);
   if (flags & NBD_REPLY_FLAG_DONE) {
-    if (cmd->type == NBD_CMD_BLOCK_STATUS && 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_BLOCK_STATUS && cmd->cb.fn.extent)
       nbd_internal_free_callback (h, cmd->cb.fn_user_data);
-    }
-    if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) {
-      cmd->cb.fn.chunk (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
-                        NULL, 0, 0, 0, NULL);
+    if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk)
       nbd_internal_free_callback (h, cmd->cb.fn_user_data);
-    }
     cmd->cb.fn.chunk = NULL;
     SET_NEXT_STATE (%^FINISH_COMMAND);
   }
diff --git a/generator/states-reply.c b/generator/states-reply.c
index d5cba1a..1e0e92c 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -173,8 +173,7 @@ save_reply_state (struct nbd_handle *h)
     int r;
 
     assert (cmd->type != NBD_CMD_DISC);
-    r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
-                            cmd->cb.user_data, &error);
+    r = cmd->cb.completion (cmd->cb.user_data, &error);
     nbd_internal_free_callback (h, cmd->cb.user_data);
     cmd->cb.completion = NULL; /* because we've freed it */
     switch (r) {
diff --git a/generator/states.c b/generator/states.c
index 313d2c9..4d3a93c 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -126,8 +126,7 @@ void abort_commands (struct nbd_handle *h,
       int r;
 
       assert (cmd->type != NBD_CMD_DISC);
-      r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
-                              cmd->cb.user_data, &error);
+      r = cmd->cb.completion (cmd->cb.user_data, &error);
       nbd_internal_free_callback (h, cmd->cb.user_data);
       cmd->cb.completion = NULL; /* because we've freed it */
       switch (r) {
diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c
index aca0564..fbd6f29 100644
--- a/interop/dirty-bitmap.c
+++ b/interop/dirty-bitmap.c
@@ -42,14 +42,11 @@ struct data {
 };
 
 static int
-cb (unsigned valid_flag, void *opaque, const char *metacontext, uint64_t
offset,
+cb (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 0b189d1..3e62b05 100644
--- a/interop/structured-read.c
+++ b/interop/structured-read.c
@@ -48,16 +48,13 @@ struct data {
 };
 
 static int
-read_cb (unsigned valid_flag, void *opaque,
+read_cb (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 8aa4547..4cc61cb 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -32,17 +32,6 @@ void
 nbd_internal_retire_and_free_command (struct nbd_handle *h,
                                       struct command *cmd)
 {
-  /* Free the callbacks. */
-  if (cmd->type == NBD_CMD_BLOCK_STATUS && 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.chunk)
-    cmd->cb.fn.chunk (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
-                      NULL, 0, 0, 0, NULL);
-  if (cmd->cb.completion)
-    cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
-                        NULL);
-
   /* Free the closures if there's an associated free callback. */
   nbd_internal_free_callback (h, cmd->cb.fn_user_data);
   nbd_internal_free_callback (h, cmd->cb.user_data);
diff --git a/lib/debug.c b/lib/debug.c
index 34d4184..bd0c465 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -42,11 +42,8 @@ int
 nbd_unlocked_set_debug_callback (struct nbd_handle *h,
                                  nbd_debug_callback debug_callback, void *data)
 {
-  if (h->debug_callback) {
-    /* ignore return value */
-    h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
+  if (h->debug_callback)
     nbd_internal_free_callback (h, h->debug_data);
-  }
   h->debug_callback = debug_callback;
   h->debug_data = data;
   return 0;
@@ -80,7 +77,7 @@ nbd_internal_debug (struct nbd_handle *h, const char *fs, ...)
 
   if (h->debug_callback)
     /* ignore return value */
-    h->debug_callback (LIBNBD_CALLBACK_VALID, h->debug_data, context,
msg);
+    h->debug_callback (h->debug_data, context, msg);
   else
     fprintf (stderr, "libnbd: debug: %s: %s: %s\n",
              h->hname, context ? : "unknown", msg);
diff --git a/lib/handle.c b/lib/handle.c
index 5a47bd8..4c59622 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -113,10 +113,8 @@ nbd_close (struct nbd_handle *h)
     return;
 
   /* Free user callbacks first. */
-  if (h->debug_callback) {
-    h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
+  if (h->debug_callback)
     nbd_internal_free_callback (h, h->debug_data);
-  }
   h->debug_callback = NULL;
 
   free (h->bs_entries);
diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c
index d8ea3f7..c1815b0 100644
--- a/tests/closure-lifetimes.c
+++ b/tests/closure-lifetimes.c
@@ -38,50 +38,59 @@ static char *nbdkit_delay[]      "delay-read=10",
     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 unsigned debug_fn_called;
+static unsigned debug_fn_freed;
+static unsigned read_cb_called;
+static unsigned read_cb_freed;
+static unsigned completion_cb_called;
+static unsigned completion_cb_freed;
 
 static int
-debug_fn (unsigned valid_flag, void *opaque,
+debug_fn (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++;
+  debug_fn_called++;
   return 0;
 }
 
+static void
+debug_fn_free (void *ptr, void *opaque)
+{
+  debug_fn_freed++;
+}
+
 static int
-read_cb (unsigned valid_flag, void *opaque,
+read_cb (void *opaque,
          const void *subbuf, size_t count,
          uint64_t offset, unsigned status, int *error)
 {
-  assert (read_cb_free == 0);
+  assert (read_cb_freed == 0);
 
-  if (valid_flag & LIBNBD_CALLBACK_VALID)
-    read_cb_valid++;
-  if (valid_flag & LIBNBD_CALLBACK_FREE)
-    read_cb_free++;
+  read_cb_called++;
   return 0;
 }
 
+static void
+read_cb_free (void *ptr, void *opaque)
+{
+  read_cb_freed++;
+}
+
 static int
-completion_cb (unsigned valid_flag, void *opaque, int *error)
+completion_cb (void *opaque, int *error)
 {
-  assert (completion_cb_free == 0);
+  assert (completion_cb_freed == 0);
 
-  if (valid_flag & LIBNBD_CALLBACK_VALID)
-    completion_cb_valid++;
-  if (valid_flag & LIBNBD_CALLBACK_FREE)
-    completion_cb_free++;
+  completion_cb_called++;
   return 0;
 }
 
+static void
+completion_cb_free (void *ptr, void *opaque)
+{
+  completion_cb_freed++;
+}
+
 #define NBD_ERROR                                               \
   do {                                                          \
     fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());    \
@@ -101,35 +110,44 @@ main (int argc, char *argv[])
   nbd = nbd_create ();
   if (nbd == NULL) NBD_ERROR;
 
-  nbd_set_debug_callback (nbd, debug_fn, NULL);
-  assert (debug_fn_free == 0);
+  if (nbd_add_free_callback (nbd, &debug_fn, debug_fn_free, NULL) == -1)
+    NBD_ERROR;
+  nbd_set_debug_callback (nbd, debug_fn, &debug_fn);
+  assert (debug_fn_freed == 0);
 
-  nbd_set_debug_callback (nbd, debug_fn, NULL);
-  assert (debug_fn_free == 1);
+  if (nbd_add_free_callback (nbd, &debug_fn, debug_fn_free, NULL) == -1)
+    NBD_ERROR;
+  nbd_set_debug_callback (nbd, debug_fn, &debug_fn);
+  assert (debug_fn_freed == 1);
 
-  debug_fn_free = 0;
+  debug_fn_freed = 0;
   nbd_close (nbd);
-  assert (debug_fn_free == 1);
+  assert (debug_fn_freed == 1);
 
   /* Test command callbacks are freed when the command is retired. */
   nbd = nbd_create ();
   if (nbd == NULL) NBD_ERROR;
   if (nbd_connect_command (nbd, nbdkit) == -1) NBD_ERROR;
 
+  if (nbd_add_free_callback (nbd, &read_cb, read_cb_free, NULL) == -1)
+    NBD_ERROR;
+  if (nbd_add_free_callback (nbd, &completion_cb, completion_cb_free, NULL)
+      == -1)
+    NBD_ERROR;
   cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0,
-                                              read_cb, NULL,
-                                              completion_cb, NULL, 0);
+                                              read_cb, &read_cb,
+                                              completion_cb,
&completion_cb, 0);
   if (cookie == -1) NBD_ERROR;
-  assert (read_cb_free == 0);
-  assert (completion_cb_free == 0);
+  assert (read_cb_freed == 0);
+  assert (completion_cb_freed == 0);
   while (!nbd_aio_command_completed (nbd, cookie)) {
     if (nbd_poll (nbd, -1) == -1) NBD_ERROR;
   }
 
-  assert (read_cb_valid == 1);
-  assert (completion_cb_valid == 1);
-  assert (read_cb_free == 1);
-  assert (completion_cb_free == 1);
+  assert (read_cb_called == 1);
+  assert (completion_cb_called == 1);
+  assert (read_cb_freed == 1);
+  assert (completion_cb_freed == 1);
 
   nbd_kill_command (nbd, 0);
   nbd_close (nbd);
@@ -137,21 +155,26 @@ main (int argc, char *argv[])
   /* Test command callbacks are freed if the handle is closed without
    * running the commands.
    */
-  read_cb_valid = read_cb_free -    completion_cb_valid = completion_cb_free =
0;
+  read_cb_called = read_cb_freed +    completion_cb_called =
completion_cb_freed = 0;
   nbd = nbd_create ();
   if (nbd == NULL) NBD_ERROR;
   if (nbd_connect_command (nbd, nbdkit_delay) == -1) NBD_ERROR;
 
+  if (nbd_add_free_callback (nbd, &read_cb, read_cb_free, NULL) == -1)
+    NBD_ERROR;
+  if (nbd_add_free_callback (nbd, &completion_cb, completion_cb_free, NULL)
+      == -1)
+    NBD_ERROR;
   cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0,
-                                              read_cb, NULL,
-                                              completion_cb, NULL, 0);
+                                              read_cb, &read_cb,
+                                              completion_cb,
&completion_cb, 0);
   if (cookie == -1) NBD_ERROR;
   nbd_kill_command (nbd, 0);
   nbd_close (nbd);
 
-  assert (read_cb_free == 1);
-  assert (completion_cb_free == 1);
+  assert (read_cb_freed == 1);
+  assert (completion_cb_freed == 1);
 
   exit (EXIT_SUCCESS);
 }
diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c
index c36a77f..0219ce9 100644
--- a/tests/meta-base-allocation.c
+++ b/tests/meta-base-allocation.c
@@ -30,7 +30,7 @@
 
 #include <libnbd.h>
 
-static int check_extent (unsigned valid_flag, void *data,
+static int check_extent (void *data,
                          const char *metacontext,
                          uint64_t offset,
                          uint32_t *entries, size_t nr_entries, int *error);
@@ -129,7 +129,7 @@ main (int argc, char *argv[])
 }
 
 static int
-check_extent (unsigned valid_flag, void *data,
+check_extent (void *data,
               const char *metacontext,
               uint64_t offset,
               uint32_t *entries, size_t nr_entries, int *error)
@@ -137,9 +137,6 @@ check_extent (unsigned valid_flag, void *data,
   size_t i;
   int id;
 
-  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
-    return 0;
-
   id = * (int *)data;
 
   printf ("extent: id=%d, metacontext=%s, offset=%" PRIu64 ",
"
diff --git a/tests/oldstyle.c b/tests/oldstyle.c
index afbda61..1382d06 100644
--- a/tests/oldstyle.c
+++ b/tests/oldstyle.c
@@ -37,15 +37,12 @@ static char wbuf[512] = { 1, 2, 3, 4 }, rbuf[512];
 static const char *progname;
 
 static int
-pread_cb (unsigned valid_flag, void *data,
+pread_cb (void *data,
           const void *buf, size_t count, uint64_t offset,
           unsigned status, int *error)
 {
   int *calls;
 
-  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
-    return 0;
-
   calls = data;
   ++*calls;
 
diff --git a/tests/server-death.c b/tests/server-death.c
index 7854527..256b45a 100644
--- a/tests/server-death.c
+++ b/tests/server-death.c
@@ -33,11 +33,8 @@ static bool trim_retired;
 static const char *progname;
 
 static int
-callback (unsigned valid_flag, void *ignored, int *error)
+callback (void *ignored, 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

On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
> This adds a C-only semi-private function for freeing various types of
> persistent data passed to libnbd.
> 
> There are some similarities with nbd_add_close_callback which we
> removed in commit 7f191b150b52ed50098976309a6af883d245fc56.
> ---
> +=head1 FREE CALLBACKS
> +
> +B<Note:> The API described in this section is only
> +available from C.  It is designed to help when writing
> +bindings to libnbd in other programming languages.
> +As such it is B<not> covered by the usual API stability
> +guarantee offered by other parts of the C API.  Use it with care.
> +
> +Some pointers passed to libnbd calls are saved in the
> +S<C<struct nbd_handle>>.  These include pointers to buffers
> +passed to C<nbd_aio_pread>, C<nbd_aio_pwrite>, etc.,
> +and pointers to the C<user_data> for callbacks.  If you
> +want to know when it is safe to free these objects then
> +you can register a free callback using:
> +
> + typedef void (*nbd_free_callback) (void *ptr, void *user_data);
> + int nbd_add_free_callback (struct nbd_handle *h,
> +                            void *ptr,
> +                            nbd_free_callback cb,
> +                            void *user_data);
Do we want to insist on a user_data argument?  Libvirt, for example,
states that free callbacks are passed only the pointer to be freed (as
you can already pack whatever you need into that pointer alone, rather
than needing yet another void *user_data); if we do not take a user_data
here, then...
> +
> +C<ptr> is a pointer to the object (ie. the buffer or
> +callback C<user_data>).  C<cb (ptr)> is called when libnbd
As written, your patch uses C<cb (ptr, user_data)>, so either this text
is wrong, or you really don't need user_data.
> +
> +where C<my_free> is a simple wrapper around the L<free(3)>
> +function (we are not using the extra C<user_data> pointer
> +in this case):
> +
> + void my_free (void *ptr, void *)
> + {
> +   free (ptr);
> + }
...without user_data, you wouldn't need a my_free wrapper, but could
just pass free() in the example.

> +++ b/lib/free.c
> @@ -0,0 +1,129 @@
> +/* This is not generated because we don't want to offer it to other
> + * programming languages.  Also it's a semi-private API liable to
> + * evolve in its exact semantics.
> + */
> +int
> +nbd_add_free_callback (struct nbd_handle *h, void *ptr,
> +                       nbd_free_callback cb, void *user_data)
At least the comment caveat gives us flexibility to change our mind on
whether to provide a user_data argument.
> +{
> +  int ret = -1;
> +  size_t i;
> +
> +  if (ptr == NULL)
> +    return 0;
> +
> +  nbd_internal_set_error_context ("nbd_add_free_callback");
> +  pthread_mutex_lock (&h->lock);
> +
> +  /* Extend the list of callbacks in the handle. */
> +  if (h->nr_free_callbacks >= h->alloc_free_callbacks) {
> +    size_t new_alloc;
> +    struct free_callback *new_callbacks;
> +
> +    if (h->alloc_free_callbacks == 0)
> +      new_alloc = 8;
> +    else
> +      new_alloc = 2 * h->alloc_free_callbacks;
> +
> +    new_callbacks = realloc (h->free_callbacks,
> +                             sizeof (struct free_callback) * new_alloc);
Should we start relying on reallocarray() to guarantee no multiplication
overflow? (Not yet in POSIX, but it has been proposed:
http://austingroupbugs.net/view.php?id=1218)
> +    if (new_callbacks == NULL) {
> +      set_error (errno, "realloc");
> +      goto out;
> +    }
> +    h->alloc_free_callbacks = new_alloc;
> +    h->free_callbacks = new_callbacks;
> +  }
> +
> +  /* Need to keep the list sorted by pointer so we can use bsearch.
> +   * Insert the new entry before the i'th entry.
> +   *
> +   * Note the same pointer can be added multiple times (eg. if a
> +   * buffer is shared between commands), and that is not a bug.  Which
> +   * free function is called is undefined, but they should all be
> +   * called eventually.
or in other words, if the free function is actually a reference counter
that only does something interesting when the count reaches 0, then
things still work.
> +   */
> +  for (i = 0; i < h->nr_free_callbacks; ++i) {
> +    if (ptr <= h->free_callbacks[i].ptr)
Comparing 2 pointers that are not allocated as part of the same array is
undefined in C.  To be safe, you're better off writing this as:

if ((intptr_t)ptr <= (intptr_t)h->free_callbacks[i].ptr)

or even storing intptr_t rather than void* in your list of pointers
needing callbacks.
> +      break;
> +  }
This is a linear search O(n) for where to insert.  Why not bsearch() for
O(log n), particularly since you document your intent to use bsearch()
for later lookups?
> +  memmove (&h->free_callbacks[i+1], &h->free_callbacks[i],
> +           (h->nr_free_callbacks - i) * sizeof (struct free_callback));
Hmm - the use of memmove() is O(n); we'd need a different data structure
(for example red-black tree or hash table) if we wanted list
manipulations to not be the chokepoint.  So, as long as you are already
stuck with an O(n) list manipulation, using O(n) lookup is not making
the problem any worse.
> +
> +  h->free_callbacks[i].ptr = ptr;
> +  h->free_callbacks[i].cb = cb;
> +  h->free_callbacks[i].user_data = user_data;
> +  h->nr_free_callbacks++;
> +
> +  ret = 0;
> + out:
> +  pthread_mutex_unlock (&h->lock);
> +  return ret;
> +}
> +
> +static int
> +compare_free_callbacks (const void *v1, const void *v2)
> +{
> +  const void *ptr = v1;
> +  const struct free_callback *cb = v2;
> +
> +  if (ptr < cb->ptr) return -1;
> +  else if (ptr > cb->ptr) return 1;
Again, undefined in C; comparing intptr_t is defined, but direct
comparison of unrelated pointers could cause a compiler to mis-optimize
(even if it happens to currently work in practice).
> +  else return 0;
> +}
> +
> +/* Called before the library frees 'ptr', where 'ptr' is
something
> + * that might potentially be associated with a free callback.  This is
> + * called often so must be fast.
> + */
> +void
> +nbd_internal_free_callback (struct nbd_handle *h, void *ptr)
> +{
> +  struct free_callback *free_cb;
> +
> +  if (ptr == NULL)
> +    return;
> +
> +  free_cb = bsearch (ptr, h->free_callbacks, h->nr_free_callbacks,
> +                     sizeof (struct free_callback),
> +                     compare_free_callbacks);
Here, you've got O(log n) lookup.
> +  if (free_cb) {
> +    assert (ptr == free_cb->ptr);
> +
> +    free_cb->cb (ptr, free_cb->user_data);
> +
> +    /* Remove it from the free list. */
> +    memmove (free_cb, free_cb+1,
> +             sizeof (struct free_callback) *
> +             (&h->free_callbacks[h->nr_free_callbacks] -
(free_cb+1)));
but then slow it down with O(n) list manipulation.

I'm still not sold on whether this is any better than our existing
completion callbacks coupled with VALID|FREE (where at least we didn't
suffer from O(n) lookups); or whether we want to instead explore taking
an explicit free function pointer as part of a C closure (that is, each
Closure maps to a three-tuple of C arguments for main function, free
function, and user_data).  But as you said in the cover letter, having
the whole series to review will let us choose between our options, so
I'll see what the rest of the series is able to accomplish with this.

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

On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
> When retiring a command test for a free_callback associated with their
> buffer.  If there is one call it.  This allows language bindings to
> use this mechanism to automatically decrement a reference to the
> persistent buffer (note: this patch does not implement this).
> 
> The vast majority of this change is simply passing around the handle
> so we have it when we call nbd_internal_free_callback in lib/aio.c.
> ---
> +++ b/lib/aio.c
> @@ -29,7 +29,8 @@
>  
>  /* Internal function which retires and frees a command. */
>  void
> -nbd_internal_retire_and_free_command (struct command *cmd)
> +nbd_internal_retire_and_free_command (struct nbd_handle *h,
> +                                      struct command *cmd)
>  {
>    /* Free the callbacks. */
>    if (cmd->type == NBD_CMD_BLOCK_STATUS &&
cmd->cb.fn.extent)
> @@ -42,6 +43,11 @@ nbd_internal_retire_and_free_command (struct command
*cmd)
>      cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
>                          NULL);
>  
> +  /* Free the persistent buffer if there is one and if there's an
> +   * associated free callback.
> +   */
> +  nbd_internal_free_callback (h, cmd->data);
> +
Looks reasonable.

However, I'm also wondering if the later patches to adjust the language
bindings can instead guarantee a C struct which includes both the buffer
(which needs decrementing) and the language-specific callback (or NULL),
where the language binding for both nbd.aio_pread and
nbd.aio_pread_complete call the underlying C nbd_aio_pread_complete, so
we don't need to manage an O(n) list of pointers with associated callbacks.

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

On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
> By using the free callback mechanism we don't need to manually free
> the buffer.
> 
> This reverts part of commit fef1c281a65d061127bf178e5f8cfca0a2475c56.
> ---
>  generator/generator              | 43 +++++++++++++++++++++-----------
>  ocaml/buffer.c                   | 22 ----------------
>  ocaml/examples/asynch_copy.ml    |  4 +--
>  ocaml/libnbd-ocaml.pod           | 22 ----------------
>  ocaml/nbd-c.h                    |  8 ++++++
>  ocaml/tests/test_590_aio_copy.ml |  4 +--
>  6 files changed, 39 insertions(+), 64 deletions(-)
> 
> diff --git a/generator/generator b/generator/generator
> index d6b9352..92ce170 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -4961,10 +4961,6 @@ module Buffer : sig
>    (** Allocate an uninitialized buffer.  The parameter is the size
>        in bytes. *)
>  
> -  val free : t -> unit
> -  (** The buffer must be manually freed when it is no longer used.
> -      An AIO completion callback is usually a good place. *)
> -
Getting rid of this public function is nice.  Hopefully, how we get rid
of it is internal enough that we can switch between either a list of
free functions at the C level, or else a smarter forwarding of aio_pread
to the C nbd_aio_pread_completion, without affecting what else leaks
through to the OCaml view.

> @@ -5244,10 +5239,8 @@ let print_ocaml_binding (name, { args; optargs; ret
}) >         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 "  if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
> +       pr "    free_root (NULL, user_data);\n";
>         pr "\n";
Here, I'm unfamiliar enough with the OCaml GC rules to know if you're
doing it right, but trust that you have been testing with valgrind or
similar with a forced GC run to catch obvious mistakes.
> @@ -5313,17 +5306,39 @@ let print_ocaml_binding (name, { args; optargs; ret
}) >      | BytesIn (n, count) ->
>         pr "  const void *%s = Bytes_val (%sv);\n" n n;
>         pr "  size_t %s = caml_string_length (%sv);\n" count n
> +    | BytesOut (n, count) ->
> +       pr "  void *%s = Bytes_val (%sv);\n" n n;
> +       pr "  size_t %s = caml_string_length (%sv);\n" count n
>      | BytesPersistIn (n, count) ->
> +       pr "  /* The function may save a reference to the Buffer, so
we\n";
> +       pr "   * must treat it as a possible GC root.\n";
> +       pr "   */\n";
> +       pr "  value *%s_user_data;\n" n;
> +       pr "  %s_user_data = malloc (sizeof (value));\n" n;
> +       pr "  if (%s_user_data == NULL) caml_raise_out_of_memory
();\n" n;
> +       pr "  *%s_user_data = %sv;\n" n n;
> +       pr "  caml_register_generational_global_root
(%s_user_data);\n" n;
>         pr "  struct nbd_buffer *%s_buf = NBD_buffer_val
(%sv);\n" n n;
>         pr "  const void *%s = %s_buf->data;\n" n n;
> -       pr "  size_t %s = %s_buf->len;\n" count n
> -    | BytesOut (n, count) ->
> -       pr "  void *%s = Bytes_val (%sv);\n" n n;
> -       pr "  size_t %s = caml_string_length (%sv);\n" count n
> +       pr "  size_t %s = %s_buf->len;\n" count n;
> +       pr "  if (nbd_add_free_callback (h, (void *)%s,\n" n;
Is the cast to void* necessary?  I guess so - it looks like you are
casting away const.
> +       pr "                             free_root, %s_user_data) ==
-1)\n" n;
Hmm. In patch 1, I questioned if we need a separate user_data argument
when registering a free callback.  But here, you are keying off of the
last use of one pointer (the buffer), but with a different pointer (the
user_data wrapper) to be passed to the free function...
> +++ b/ocaml/nbd-c.h
> @@ -107,4 +107,12 @@ struct callback_data {
>  extern char **nbd_internal_ocaml_string_list (value);
>  extern value nbd_internal_ocaml_alloc_int32_array (uint32_t *, size_t);
>  
> +/* Free a generational global root and its container. */
> +static inline void
> +free_root (void *ptr /* unused */, void *rootp)
> +{
> +  caml_remove_generational_global_root ((value *)rootp);
> +  free (rootp);
> +}
...and indeed, the free function is ignoring the first pointer and
instead acting on the second.  If we change the C signature to take only
a single pointer, then you have to be a bit more creative on h ow to
pass the rootp as the pointer to be adjusted when the buffer lifetime
ends.  So maybe this is the answer to my question in patch 1 as to why
you needed two arguments. Or maybe it is all the more reason to try
harder to get the generator to call nbd_aio_pread_callback even when the
OCaml language did not pass a callback.

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

On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
> Mechanical change: Wherever we call any closure with the
> LIBNBD_CALLBACK_FREE function, we also call nbd_internal_free_callback
> with the closure's user_data.  This allows calls to associate a free
> callback with any closure via its user_data pointer.
> ---
> +++ b/lib/aio.c
> @@ -43,6 +43,10 @@ nbd_internal_retire_and_free_command (struct nbd_handle
*h,
>      cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
>                          NULL);
>  
> +  /* Free the closures if there's an associated free callback. */
> +  nbd_internal_free_callback (h, cmd->cb.fn_user_data);
This calls the fn_user_data callback too many times if it was already
reached.  Instead, you need:

if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent) {
  cmd->cb.fn.extent (_FREE...);
  nbd_internal_free_callback (h, cmd->cb.fn_user_data);
}
else if (cmd->type == NBD_CMD_BLOCK_READ && cmd->cb.fn.chunk) {
  cmd->cb.fn.chunk (_FREE...);
  nbd_internal_free_callback (h, cmd->cb.fn_user_data);
}

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

On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
> Instead of using the valid_flag == FREE mechanism, use a free callback
> to free each closure root.
> ---
>  generator/generator | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
Looks like a straight swap; at this point in the series, valid_flag &
FREE is called exactly as many times as a free callback; and moving to a
free callback would mean a later patch could get rid of valid_flag if we
wanted (although that's a C API change).

Still, if we're able to reliably call the C nbd_aio_pread_callback even
from the OCaml nbd.aio_pread, then the valid_flag = FREE mechanism
results in fewer function calls than having to register a free callback
for each pointer to be cleaned up.

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

On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
> Same as the previous commit, but for Python.
> ---
>  generator/generator | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> @@ -4454,6 +4451,11 @@ let print_python_binding name { args; optargs; ret;
may_set_error } >         pr "    PyErr_SetString
(PyExc_TypeError,\n";
>         pr "                     \"callback parameter %s is not
callable\");\n" cbname;
>         pr "    return NULL;\n";
> +       pr "  }\n";
> +       pr "  if (nbd_add_free_callback (h, %s_user_data,\n"
cbname;
> +       pr "                             decref, NULL) == -1)
{\n";
> +       pr "    PyErr_NoMemory ();\n";
> +       pr "    return NULL;\n";
>         pr "  }\n"
>      | Enum _ -> ()
>      | Flags (n, _) -> pr "  %s_u32 = %s;\n" n n
> @@ -4600,6 +4602,12 @@ let generate_python_methods_c () >    pr
"\n";
>    pr "#include <methods.h>\n";
>    pr "\n";
> +  pr "static void\n";
> +  pr "decref (void *ptr, void *user_data)\n";
> +  pr "{\n";
> +  pr "  Py_DECREF (ptr);\n";
> +  pr "}\n";
Well, in the OCaml case, you were actively using the fact that ptr and
user_data might be different, and freeing the second; where in the
Python case you are only ever caring about the ptr argument and always
passing NULL for the second (meaning you had to write a shim around
Py_DecRef, as hidden under the Py_DECREF macro).

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

On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
> The freeing feature can now be done by associating a free callback
> with a closure's user_data, so having the valid_flag is no longer
> necessary and it can be completely removed.
> 
> This mostly reverts commit 2d9b98e96772e282d51dafac07f16387dadc8afa.
Aha - the end goal is to get rid of valid_flag. But still, we might be
able to do that by instead having C expand Closure into a 3-tuple
(callback, free_cb, user_data) where we call callback(user_data, args)
as needed and then free_cb(user_data) when done, rather than having to
add a list of free callback registrations.

Definitely an API/ABI change (the fio code using the existing interface
will need tweaking), but seems like a worthwhile goal, even if we change
our minds about how best to get to this point.

I'll have to play more with the ideas of making the generator smarter
about using underlying nbd_aio_pread_callback even for the public
Python/OCaml nbd.nbd_aio_pread as a way to create a C structure that can
call free_cb(wrapper) to clean up the lifetime of both the buffer and
any language callback reference.

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