Libguestfs - Aug 2019 - [PATCH libnbd v2 04/10] lib: Permit .callback = NULL, .free != NULL.

This is a combination of these two earlier series:

https://www.redhat.com/archives/libguestfs/2019-August/msg00235.html
https://www.redhat.com/archives/libguestfs/2019-August/msg00240.html

plus changes to allow .callback = NULL / .free != NULL, and to reduce
the complexity of freeing callbacks.

Although it's rather long there's nothing complex here.  We might
consider squashing some patches together.

I believe I've addressed everything raised in the earlier reviews,
with the exception that I'm not using Py_XDECREF because it seems a
bit obscure (I'd never heard of it myself and it hides the true
meaning of the statement).

Rich.

Earlier on we had the ‘valid_flag’.  It was possible to set this to
VALID|FREE avoiding an additional function call.  Since we got rid of
this flag in favour of two separate functions there is no possible way
to avoid an extra function call, and freeing the callbacks close to
the point of last use is therefore no longer worthwhile.  Instead free
all command callbacks in one place
(lib/aio.c:nbd_internal_retire_and_free_command).

Thanks: Eric Blake for pointing this out.
---
 generator/states-reply-simple.c     |  1 -
 generator/states-reply-structured.c | 17 -----------------
 generator/states-reply.c            |  1 -
 generator/states.c                  |  1 -
 4 files changed, 20 deletions(-)

diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 8e3d7f1..2f83e6f 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -69,7 +69,6 @@
                          cmd->offset, LIBNBD_READ_DATA,
                          &error) == -1)
         cmd->error = error ? error : EPROTO;
-      FREE_CALLBACK (cmd->cb.fn.chunk);
     }
 
     SET_NEXT_STATE (%^FINISH_COMMAND);
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 2e327ce..a1641d4 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -295,7 +295,6 @@
       }
       if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback)
{
         int scratch = error;
-        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
@@ -307,8 +306,6 @@
                            &scratch) == -1)
           if (cmd->error == 0)
             cmd->error = scratch;
-        if (flags & NBD_REPLY_FLAG_DONE)
-          FREE_CALLBACK (cmd->cb.fn.chunk);
       }
     }
 
@@ -390,15 +387,12 @@
     assert (cmd); /* guaranteed by CHECK */
     if (cmd->cb.fn.chunk.callback) {
       int error = cmd->error;
-      uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
 
       if (CALL_CALLBACK (cmd->cb.fn.chunk, cmd->data + (offset -
cmd->offset),
                          length - sizeof offset, offset,
                          LIBNBD_READ_DATA, &error) == -1)
         if (cmd->error == 0)
           cmd->error = error ? error : EPROTO;
-      if (flags & NBD_REPLY_FLAG_DONE)
-        FREE_CALLBACK (cmd->cb.fn.chunk);
     }
 
     SET_NEXT_STATE (%FINISH);
@@ -454,7 +448,6 @@
     memset (cmd->data + offset, 0, length);
     if (cmd->cb.fn.chunk.callback) {
       int error = cmd->error;
-      uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
 
       if (CALL_CALLBACK (cmd->cb.fn.chunk,
                          cmd->data + offset, length,
@@ -462,8 +455,6 @@
                          LIBNBD_READ_HOLE, &error) == -1)
         if (cmd->error == 0)
           cmd->error = error ? error : EPROTO;
-      if (flags & NBD_REPLY_FLAG_DONE)
-        FREE_CALLBACK (cmd->cb.fn.chunk);
     }
 
     SET_NEXT_STATE(%FINISH);
@@ -509,7 +500,6 @@
     if (meta_context) {
       /* Call the caller's extent function. */
       int error = cmd->error;
-      uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
 
       if (CALL_CALLBACK (cmd->cb.fn.extent,
                          meta_context->name, cmd->offset,
@@ -517,8 +507,6 @@
                          &error) == -1)
         if (cmd->error == 0)
           cmd->error = error ? error : EPROTO;
-      if (flags & NBD_REPLY_FLAG_DONE)
-        FREE_CALLBACK (cmd->cb.fn.extent);
     }
     else
       /* Emit a debug message, but ignore it. */
@@ -530,15 +518,10 @@
   return 0;
 
  REPLY.STRUCTURED_REPLY.FINISH:
-  struct command *cmd = h->reply_cmd;
   uint16_t flags;
 
   flags = be16toh (h->sbuf.sr.structured_reply.flags);
   if (flags & NBD_REPLY_FLAG_DONE) {
-    if (cmd->type == NBD_CMD_BLOCK_STATUS)
-      FREE_CALLBACK (cmd->cb.fn.extent);
-    if (cmd->type == NBD_CMD_READ)
-      FREE_CALLBACK (cmd->cb.fn.chunk);
     SET_NEXT_STATE (%^FINISH_COMMAND);
   }
   else {
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 41dcf8d..575a6d1 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -174,7 +174,6 @@ save_reply_state (struct nbd_handle *h)
 
     assert (cmd->type != NBD_CMD_DISC);
     r = CALL_CALLBACK (cmd->cb.completion, &error);
-    FREE_CALLBACK (cmd->cb.completion);
     switch (r) {
     case -1:
       if (error)
diff --git a/generator/states.c b/generator/states.c
index 263f60e..cfa9957 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -127,7 +127,6 @@ void abort_commands (struct nbd_handle *h,
 
       assert (cmd->type != NBD_CMD_DISC);
       r = CALL_CALLBACK (cmd->cb.completion, &error);
-      FREE_CALLBACK (cmd->cb.completion);
       switch (r) {
       case -1:
         if (error)
-- 
2.22.0

We have defined the concept of a "null callback" meaning one where the
.callback field = NULL.  The first two new macros just test this
property, and the third one sets a callback to null.

This change is neutral refactoring.
---
 generator/generator                 |  4 ++--
 generator/states-reply-simple.c     |  2 +-
 generator/states-reply-structured.c | 11 ++++++-----
 generator/states-reply.c            |  2 +-
 generator/states.c                  |  2 +-
 lib/debug.c                         |  2 +-
 lib/internal.h                      |  9 +++++++--
 7 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/generator/generator b/generator/generator
index ca97910..98c99e0 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3576,7 +3576,7 @@ let generate_lib_api_c ()           let value = match
errcode with
            | Some value -> value
            | None -> assert false in
-         pr "  if (%s_callback.callback == NULL) {\n" cbname;
+         pr "  if (CALLBACK_IS_NULL (%s_callback)) {\n" cbname;
          pr "    set_error (EFAULT, \"%%s cannot be NULL\",
\"%s\");\n" cbname;
          pr "    ret = %s;\n" value;
          pr "    goto out;\n";
@@ -3689,7 +3689,7 @@ let generate_lib_api_c ()      List.iter (
       function
       | OClosure { cbname } ->
-         pr ", %s_callback.callback ? \"<fun>\" :
\"NULL\"" cbname
+         pr ", CALLBACK_IS_NULL (%s_callback) ? \"<fun>\"
: \"NULL\"" cbname
       | OFlags (n, _) -> pr ", %s" n
     ) optargs;
     pr ");\n"
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 2f83e6f..5e5b631 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -60,7 +60,7 @@
   case 0:
     /* guaranteed by START */
     assert (cmd);
-    if (cmd->cb.fn.chunk.callback) {
+    if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
       int error = 0;
 
       assert (cmd->error == 0);
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index a1641d4..85f0775 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -157,7 +157,7 @@
       set_error (0, "invalid length in NBD_REPLY_TYPE_BLOCK_STATUS");
       return 0;
     }
-    if (cmd->cb.fn.extent.callback == NULL) {
+    if (CALLBACK_IS_NULL (cmd->cb.fn.extent)) {
       SET_NEXT_STATE (%.DEAD);
       set_error (0, "not expecting NBD_REPLY_TYPE_BLOCK_STATUS
here");
       return 0;
@@ -293,7 +293,8 @@
                    offset, cmd->offset, cmd->count);
         return 0;
       }
-      if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback)
{
+      if (cmd->type == NBD_CMD_READ &&
+          CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
         int scratch = error;
 
         /* Different from successful reads: inform the callback about the
@@ -385,7 +386,7 @@
     offset = be64toh (h->sbuf.sr.payload.offset_data.offset);
 
     assert (cmd); /* guaranteed by CHECK */
-    if (cmd->cb.fn.chunk.callback) {
+    if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
       int error = cmd->error;
 
       if (CALL_CALLBACK (cmd->cb.fn.chunk, cmd->data + (offset -
cmd->offset),
@@ -446,7 +447,7 @@
      * them as an extension, and this works even when length == 0.
      */
     memset (cmd->data + offset, 0, length);
-    if (cmd->cb.fn.chunk.callback) {
+    if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
       int error = cmd->error;
 
       if (CALL_CALLBACK (cmd->cb.fn.chunk,
@@ -479,7 +480,7 @@
 
     assert (cmd); /* guaranteed by CHECK */
     assert (cmd->type == NBD_CMD_BLOCK_STATUS);
-    assert (cmd->cb.fn.extent.callback);
+    assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent));
     assert (h->bs_entries);
     assert (length >= 12);
 
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 575a6d1..14bb010 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -168,7 +168,7 @@ save_reply_state (struct nbd_handle *h)
   retire = cmd->type == NBD_CMD_DISC;
 
   /* Notify the user */
-  if (cmd->cb.completion.callback) {
+  if (CALLBACK_IS_NOT_NULL (cmd->cb.completion)) {
     int error = cmd->error;
     int r;
 
diff --git a/generator/states.c b/generator/states.c
index cfa9957..32bf975 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -121,7 +121,7 @@ void abort_commands (struct nbd_handle *h,
     bool retire = cmd->type == NBD_CMD_DISC;
 
     next = cmd->next;
-    if (cmd->cb.completion.callback) {
+    if (CALLBACK_IS_NOT_NULL (cmd->cb.completion)) {
       int error = cmd->error ? cmd->error : ENOTCONN;
       int r;
 
diff --git a/lib/debug.c b/lib/debug.c
index eec2051..90d2f71 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -82,7 +82,7 @@ nbd_internal_debug (struct nbd_handle *h, const char *fs, ...)
   if (r == -1)
     goto out;
 
-  if (h->debug_callback.callback)
+  if (CALLBACK_IS_NOT_NULL (h->debug_callback))
     /* ignore return value */
     CALL_CALLBACK (h->debug_callback, context, msg);
   else
diff --git a/lib/internal.h b/lib/internal.h
index dc3676a..a18d581 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -273,6 +273,11 @@ struct command {
   uint32_t error; /* Local errno value */
 };
 
+/* Test if a callback is "null" or not, and set it to null. */
+#define CALLBACK_IS_NULL(cb)     ((cb).callback == NULL)
+#define CALLBACK_IS_NOT_NULL(cb) (! CALLBACK_IS_NULL ((cb)))
+#define SET_CALLBACK_TO_NULL(cb) ((cb).callback = NULL)
+
 /* Call a callback. */
 #define CALL_CALLBACK(cb, ...) \
   (cb).callback ((cb).user_data, ##__VA_ARGS__)
@@ -286,9 +291,9 @@ struct command {
 #define FREE_CALLBACK(cb)                                               \
   do {                                                                  \
     nbd_completion_callback *_cb = (nbd_completion_callback *)&(cb);    \
-    if (_cb->callback != NULL && _cb->free != NULL)              
\
+    if (CALLBACK_IS_NOT_NULL (cb) && _cb->free != NULL)             
\
       _cb->free (_cb->user_data);                                       \
-    _cb->callback = NULL;                                               \
+    SET_CALLBACK_TO_NULL (cb);                                          \
   } while (0)
 
 /* aio.c */
-- 
2.22.0

Why did I need to cast cb to a particular type?  I don't know.

Fixes commit 9c8fccdf382c2c8483b557acc6b5d41a4e193990.
---
 lib/internal.h | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h
index a18d581..1344d98 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -282,17 +282,11 @@ struct command {
 #define CALL_CALLBACK(cb, ...) \
   (cb).callback ((cb).user_data, ##__VA_ARGS__)
 
-/* Free a callback.
- *
- * Note this works for any type of callback because the basic layout
- * of the struct is the same for all of them.  Therefore casting cb to
- * nbd_completion_callback does not change the effective code.
- */
+/* Free a callback. */
 #define FREE_CALLBACK(cb)                                               \
   do {                                                                  \
-    nbd_completion_callback *_cb = (nbd_completion_callback *)&(cb);    \
-    if (CALLBACK_IS_NOT_NULL (cb) && _cb->free != NULL)             
\
-      _cb->free (_cb->user_data);                                       \
+    if (CALLBACK_IS_NOT_NULL (cb) && (cb).free != NULL)                
\
+      (cb).free ((cb).user_data);                                       \
     SET_CALLBACK_TO_NULL (cb);                                          \
   } while (0)
 
-- 
2.22.0

Previously the .free function of a callback was not called if the
.callback field was NULL, because the callback as a whole would be
considered to be "null".

This change allows you to register callbacks where the .callback field
is NULL, but the .free field is != NULL, meaning that the callback is
freed after the last time it would have been used.

This is mainly convenient for language bindings where we sometimes
want to register a free function to clean up a persistent buffer, but
we don't need the associated completion callback to be actually
called.
---
 docs/libnbd.pod | 15 +++++++++++++++
 lib/internal.h  | 18 +++++++++---------
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index f6fd4cd..d230cb4 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -643,6 +643,21 @@ S<C<chunk.callback = my_fn>> function is
called.
 The free function is only accessible in the C API as it is not needed
 in garbage collected programming languages.
 
+=head2 Callbacks with C<.callback=NULL> and C<.free!=NULL>
+
+It is possible to register a callback like this:
+
+  ...
+    (nbd_completion_callback) { .callback = NULL,
+                                .user_data = my_data,
+                                .free = free },
+  ...
+
+The meaning of this is that the callback is never called, but the free
+function is still called after the last time the callback would have
+been called.  This is useful for applying generic freeing actions when
+asynchronous commands are retired.
+
 =head2 Callbacks and locking
 
 The callbacks are invoked at a point where the libnbd lock is held; as
diff --git a/lib/internal.h b/lib/internal.h
index 1344d98..ee59582 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -274,20 +274,20 @@ struct command {
 };
 
 /* Test if a callback is "null" or not, and set it to null. */
-#define CALLBACK_IS_NULL(cb)     ((cb).callback == NULL)
+#define CALLBACK_IS_NULL(cb)     ((cb).callback == NULL && (cb).free ==
NULL)
 #define CALLBACK_IS_NOT_NULL(cb) (! CALLBACK_IS_NULL ((cb)))
-#define SET_CALLBACK_TO_NULL(cb) ((cb).callback = NULL)
+#define SET_CALLBACK_TO_NULL(cb) ((cb).callback = NULL, (cb).free = NULL)
 
 /* Call a callback. */
-#define CALL_CALLBACK(cb, ...) \
-  (cb).callback ((cb).user_data, ##__VA_ARGS__)
+#define CALL_CALLBACK(cb, ...)                                          \
+  ((cb).callback != NULL ? (cb).callback ((cb).user_data, ##__VA_ARGS__) : 0)
 
 /* Free a callback. */
-#define FREE_CALLBACK(cb)                                               \
-  do {                                                                  \
-    if (CALLBACK_IS_NOT_NULL (cb) && (cb).free != NULL)                
\
-      (cb).free ((cb).user_data);                                       \
-    SET_CALLBACK_TO_NULL (cb);                                          \
+#define FREE_CALLBACK(cb)                               \
+  do {                                                  \
+    if ((cb).free != NULL)                              \
+      (cb).free ((cb).user_data);                       \
+    SET_CALLBACK_TO_NULL (cb);                          \
   } while (0)
 
 /* aio.c */
-- 
2.22.0

Just a simple refactoring.
---
 generator/generator | 71 +++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 28 deletions(-)

diff --git a/generator/generator b/generator/generator
index 98c99e0..35936c6 100755
--- a/generator/generator
+++ b/generator/generator
@@ -4945,7 +4945,8 @@ let print_ocaml_closure_wrapper { cbname; cbargs }   
assert (List.length argnames <= 5);
   pr "  CAMLlocal%d (%s);\n" (List.length argnames)
     (String.concat ", " argnames);
-  pr "  CAMLlocal3 (fnv, exn, rv);\n";
+  pr "  CAMLlocal2 (exn, rv);\n";
+  pr "  const struct user_data *data = user_data;\n";
   pr "  int r;\n";
   pr "  value args[%d];\n" (List.length argnames);
   pr "\n";
@@ -4974,9 +4975,7 @@ let print_ocaml_closure_wrapper { cbname; cbargs }  
   List.iteri (fun i n -> pr "  args[%d] = %s;\n" i n) argnames;
 
-  pr "  fnv = * (value *) user_data;\n";
-
-  pr "  rv = caml_callbackN_exn (fnv, %d, args);\n"
+  pr "  rv = caml_callbackN_exn (data->fnv, %d, args);\n"
     (List.length argnames);
 
   List.iter (
@@ -5039,15 +5038,7 @@ let print_ocaml_closure_wrapper { cbname; cbargs }    pr
";\n";
   pr "  caml_enter_blocking_section ();\n";
   pr "  return ret;\n";
-  pr "}\n";
-
-  pr "static void\n";
-  pr "%s_free (void *user_data)\n" cbname;
-  pr "{\n";
-  pr "  caml_remove_generational_global_root (user_data);\n";
-  pr "  free (user_data);\n";
-  pr "}\n";
-  pr "\n"
+  pr "}\n"
 
 let print_ocaml_binding (name, { args; optargs; ret })    (* Get the names of
all the value arguments including the handle. *)
@@ -5098,14 +5089,13 @@ let print_ocaml_binding (name, { args; optargs; ret })  
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";
-       pr "    %s_callback.user_data = malloc (sizeof (value));\n"
cbname;
-       pr "    if (%s_callback.user_data == NULL)\n" cbname;
-       pr "      caml_raise_out_of_memory ();\n";
-       pr "    *(value *)%s_callback.user_data = Field (%sv, 0);\n"
-         cbname cbname;
-       pr "    caml_register_generational_global_root
(%s_callback.user_data);\n" cbname;
+       pr "    struct user_data *user_data = alloc_user_data ();\n";
+       pr "\n";
+       pr "    user_data->fnv = Field (%sv, 0);\n" cbname;
+       pr "    caml_register_generational_global_root
(&user_data->fnv);\n";
        pr "    %s_callback.callback = %s_wrapper;\n" cbname cbname;
-       pr "    %s_callback.free = %s_free;\n" cbname cbname;
+       pr "    %s_callback.user_data = user_data;\n" cbname;
+       pr "    %s_callback.free = free_user_data;\n" cbname;
        pr "  }\n";
     | OFlags (n, { flag_prefix }) ->
        pr "  uint32_t %s;\n" n;
@@ -5138,14 +5128,13 @@ let print_ocaml_binding (name, { args; optargs; ret })  
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";
-       pr "  nbd_%s_callback %s_callback = { .callback = %s_wrapper, .free
= %s_free };\n"
-         cbname cbname cbname cbname;
-       pr "  %s_callback.user_data = malloc (sizeof (value));\n"
cbname;
-       pr "  if (%s_callback.user_data == NULL) caml_raise_out_of_memory
();\n"
-         cbname;
-       pr "  *(value *)%s_callback.user_data = %sv;\n" cbname cbname;
-       pr "  caml_register_generational_global_root
(%s_callback.user_data);\n"
-         cbname
+       pr "  struct user_data *user_data = alloc_user_data ();\n";
+       pr "  user_data->fnv = %sv;\n" cbname;
+       pr "  caml_register_generational_global_root
(&user_data->fnv);\n";
+       pr "  nbd_%s_callback %s_callback;\n" cbname cbname;
+       pr "  %s_callback.callback = %s_wrapper;\n" cbname cbname;
+       pr "  %s_callback.user_data = user_data;\n" cbname;
+       pr "  %s_callback.free = free_user_data;\n" cbname
     | Enum (n, { enum_prefix }) ->
        pr "  int %s = %s_val (%sv);\n" n enum_prefix n
     | Flags (n, { flag_prefix }) ->
@@ -5264,6 +5253,32 @@ let generate_ocaml_nbd_c ()    pr "#pragma GCC
diagnostic ignored \"-Wmissing-prototypes\"\n";
   pr "\n";
 
+  pr "/* This is passed to *_wrapper as the user_data pointer";
+  pr " * and freed in the free_user_data function below.\n";
+  pr " */\n";
+  pr "struct user_data {\n";
+  pr "  value fnv;      /* GC root pointing to OCaml function. */\n";
+  pr "};\n";
+  pr "\n";
+  pr "static struct user_data *\n";
+  pr "alloc_user_data (void)\n";
+  pr "{\n";
+  pr "  struct user_data *data = calloc (1, sizeof *data);\n";
+  pr "  if (data == NULL)\n";
+  pr "    caml_raise_out_of_memory ();\n";
+  pr "  return data;\n";
+  pr "}\n";
+  pr "\n";
+  pr "static void\n";
+  pr "free_user_data (void *user_data)\n";
+  pr "{\n";
+  pr "  struct user_data *data = user_data;\n";
+  pr "\n";
+  pr "  caml_remove_generational_global_root (&data->fnv);\n";
+  pr "  free (data);\n";
+  pr "}\n";
+  pr "\n";
+
   List.iter print_ocaml_closure_wrapper all_closures;
   List.iter print_ocaml_enum_val all_enums;
   List.iter print_ocaml_flag_val all_flags;
-- 
2.22.0

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

This reverts part of commit fef1c281a65d061127bf178e5f8cfca0a2475c56.
---
 generator/generator              | 49 ++++++++++++++++++++------------
 ocaml/buffer.c                   | 22 --------------
 ocaml/examples/asynch_copy.ml    |  4 +--
 ocaml/libnbd-ocaml.pod           | 22 --------------
 ocaml/tests/test_590_aio_copy.ml |  4 +--
 5 files changed, 33 insertions(+), 68 deletions(-)

diff --git a/generator/generator b/generator/generator
index 35936c6..f307485 100755
--- a/generator/generator
+++ b/generator/generator
@@ -4747,10 +4747,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. *)
 
@@ -4848,7 +4844,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\"
@@ -5085,18 +5080,18 @@ let print_ocaml_binding (name, { args; optargs; ret })  
function
     | OClosure { cbname } ->
        pr "  nbd_%s_callback %s_callback = {0};\n" cbname cbname;
+       pr "  struct user_data *%s_user_data = alloc_user_data ();\n"
cbname;
        pr "  if (%sv != Val_int (0)) { /* Some closure */\n" 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";
-       pr "    struct user_data *user_data = alloc_user_data ();\n";
-       pr "\n";
-       pr "    user_data->fnv = Field (%sv, 0);\n" cbname;
-       pr "    caml_register_generational_global_root
(&user_data->fnv);\n";
+       pr "    %s_user_data->fnv = Field (%sv, 0);\n" cbname
cbname;
+       pr "    caml_register_generational_global_root
(&%s_user_data->fnv);\n"
+         cbname;
        pr "    %s_callback.callback = %s_wrapper;\n" cbname cbname;
-       pr "    %s_callback.user_data = user_data;\n" cbname;
-       pr "    %s_callback.free = free_user_data;\n" cbname;
        pr "  }\n";
+       pr "  %s_callback.user_data = %s_user_data;\n" cbname cbname;
+       pr "  %s_callback.free = free_user_data;\n" cbname;
     | OFlags (n, { flag_prefix }) ->
        pr "  uint32_t %s;\n" n;
        pr "  if (%sv != Val_int (0)) /* Some [ list of %s.t ] */\n"
@@ -5125,15 +5120,16 @@ let print_ocaml_binding (name, { args; optargs; ret })  
pr "  void *%s = %s_buf->data;\n" n n;
        pr "  size_t %s = %s_buf->len;\n" count n
     | Closure { cbname } ->
+       pr "  nbd_%s_callback %s_callback;\n" cbname cbname;
+       pr "  struct user_data *%s_user_data = alloc_user_data ();\n"
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";
-       pr "  struct user_data *user_data = alloc_user_data ();\n";
-       pr "  user_data->fnv = %sv;\n" cbname;
-       pr "  caml_register_generational_global_root
(&user_data->fnv);\n";
-       pr "  nbd_%s_callback %s_callback;\n" cbname cbname;
+       pr "  %s_user_data->fnv = %sv;\n" cbname cbname;
+       pr "  caml_register_generational_global_root
(&%s_user_data->fnv);\n"
+         cbname;
        pr "  %s_callback.callback = %s_wrapper;\n" cbname cbname;
-       pr "  %s_callback.user_data = user_data;\n" cbname;
+       pr "  %s_callback.user_data = %s_user_data;\n" cbname cbname;
        pr "  %s_callback.free = free_user_data;\n" cbname
     | Enum (n, { enum_prefix }) ->
        pr "  int %s = %s_val (%sv);\n" n enum_prefix n
@@ -5159,6 +5155,19 @@ let print_ocaml_binding (name, { args; optargs; ret })   
pr "  uint64_t %s = Int64_val (%sv);\n" n n
   ) args;
 
+  (* If there is a BytesPersistIn/Out parameter then we need to
+   * register it as a global root and save that into the
+   * completion_callback.user_data so the root is removed on
+   * command completion.
+   *)
+  List.iter (
+    function
+    | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
+       pr "  completion_user_data->bufv = %sv;\n" n;
+       pr "  caml_register_generational_global_root
(&completion_user_data->bufv);\n"
+    | _ -> ()
+  ) args;
+
   let ret_c_type = C.type_of_ret ret and errcode = C.errcode_of_ret ret in
   pr "  %s r;\n" ret_c_type;
   pr "\n";
@@ -5257,7 +5266,8 @@ let generate_ocaml_nbd_c ()    pr " * and freed in
the free_user_data function below.\n";
   pr " */\n";
   pr "struct user_data {\n";
-  pr "  value fnv;      /* GC root pointing to OCaml function. */\n";
+  pr "  value fnv;     /* Optional GC root pointing to OCaml function.
*/\n";
+  pr "  value bufv;    /* Optional GC root pointing to persistent buffer.
*/\n";
   pr "};\n";
   pr "\n";
   pr "static struct user_data *\n";
@@ -5274,7 +5284,10 @@ let generate_ocaml_nbd_c ()    pr "{\n";
   pr "  struct user_data *data = user_data;\n";
   pr "\n";
-  pr "  caml_remove_generational_global_root (&data->fnv);\n";
+  pr "  if (data->fnv != 0)\n";
+  pr "    caml_remove_generational_global_root
(&data->fnv);\n";
+  pr "  if (data->bufv != 0)\n";
+  pr "    caml_remove_generational_global_root
(&data->bufv);\n";
   pr "  free (data);\n";
   pr "}\n";
   pr "\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 8057118..d5dcc60 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 d370113..ae8fb88 100644
--- a/ocaml/libnbd-ocaml.pod
+++ b/ocaml/libnbd-ocaml.pod
@@ -33,28 +33,6 @@ which is the printable error message.  The second is the raw
C<errno>,
 if available (see L<nbd_get_errno(3)>).  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/tests/test_590_aio_copy.ml b/ocaml/tests/test_590_aio_copy.ml
index defb4cb..ac490ef 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

Simple refactoring to use a struct to store the function pointer
passed to the callback wrapper.
---
 generator/generator | 69 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/generator/generator b/generator/generator
index f307485..8c055a9 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3979,6 +3979,7 @@ let print_python_closure_wrapper { cbname; cbargs }   
C.print_cbarg_list cbargs;
   pr "\n";
   pr "{\n";
+  pr "  const struct user_data *data = user_data;\n";
   pr "  int ret = 0;\n";
   pr "\n";
   pr "  PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
@@ -4037,7 +4038,7 @@ let print_python_closure_wrapper { cbname; cbargs }    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 (data->fn, py_args);\n";
   pr "\n";
   pr "  if (PyEval_ThreadsInitialized ())\n";
   pr "    PyGILState_Release (py_save);\n";
@@ -4079,13 +4080,6 @@ let print_python_closure_wrapper { cbname; cbargs }    )
cbargs;
   pr "  return ret;\n";
   pr "}\n";
-  pr "\n";
-  pr "/* Free for %s callback. */\n" cbname;
-  pr "static void\n";
-  pr "%s_free (void *user_data)\n" cbname;
-  pr "{\n";
-  pr "  Py_DECREF (user_data);\n";
-  pr "}\n";
   pr "\n"
 
 (* Generate the Python binding. *)
@@ -4111,8 +4105,12 @@ let print_python_binding name { args; optargs; ret;
may_set_error }            n;
        pr "  struct py_aio_buffer *%s_buf;\n" n
     | Closure { cbname } ->
-       pr "  nbd_%s_callback %s = { .callback = %s_wrapper, .free =
%s_free };\n"
-         cbname cbname cbname cbname
+       pr "  struct user_data *%s_user_data = alloc_user_data ();\n"
cbname;
+       pr "  if (%s_user_data == NULL) return NULL;\n" cbname;
+       pr "  nbd_%s_callback %s = { .callback = %s_wrapper,\n"
+         cbname cbname cbname;
+       pr "                         .user_data = %s_user_data,\n"
cbname;
+       pr "                         .free = free_user_data };\n"
     | Enum (n, _) -> pr "  int %s;\n" n
     | Flags (n, _) ->
        pr "  uint32_t %s_u32;\n" n;
@@ -4144,8 +4142,12 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    List.iter (
     function
     | OClosure { cbname } ->
-       pr "  nbd_%s_callback %s = { .callback = %s_wrapper, .free =
%s_free };\n"
-         cbname cbname cbname cbname
+       pr "  struct user_data *%s_user_data = alloc_user_data ();\n"
cbname;
+       pr "  if (%s_user_data == NULL) return NULL;\n" cbname;
+       pr "  nbd_%s_callback %s = { .callback = %s_wrapper,\n"
+         cbname cbname cbname;
+       pr "                         .user_data = %s_user_data,\n"
cbname;
+       pr "                         .free = free_user_data };\n"
     | OFlags (n, _) ->
        pr "  uint32_t %s_u32;\n" n;
        pr "  unsigned int %s; /* really uint32_t */\n" n
@@ -4188,7 +4190,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }      | 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->fn" cbname
     | Enum (n, _) -> pr ", &%s" n
     | Flags (n, _) -> pr ", &%s" n
     | Int n -> pr ", &%s" n
@@ -4203,7 +4205,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
-    | OClosure { cbname } -> pr ", &%s.user_data" cbname
+    | OClosure { cbname } -> pr ", &%s_user_data->fn"
cbname
     | OFlags (n, _) -> pr ", &%s" n
   ) optargs;
   pr "))\n";
@@ -4220,8 +4222,8 @@ let print_python_binding name { args; optargs; ret;
may_set_error }         pr "  %s_buf = nbd_internal_py_get_aio_buffer
(%s);\n" n n
     | 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 "  Py_INCREF (%s_user_data->fn);\n" cbname;
+       pr "  if (!PyCallable_Check (%s_user_data->fn)) {\n"
cbname;
        pr "    PyErr_SetString (PyExc_TypeError,\n";
        pr "                     \"callback parameter %s is not
callable\");\n" cbname;
        pr "    return NULL;\n";
@@ -4246,10 +4248,10 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    List.iter (
     function
     | OClosure { cbname } ->
-       pr "  if (%s.user_data != Py_None) {\n" cbname;
+       pr "  if (%s_user_data->fn != Py_None) {\n" 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 "    Py_INCREF (%s_user_data->fn);\n" cbname;
+       pr "    if (!PyCallable_Check (%s_user_data->fn)) {\n"
cbname;
        pr "      PyErr_SetString (PyExc_TypeError,\n";
        pr "                       \"callback parameter %s is not
callable\");\n" cbname;
        pr "      return NULL;\n";
@@ -4382,6 +4384,35 @@ let generate_python_methods_c ()    pr "\n";
   pr "#include <methods.h>\n";
   pr "\n";
+
+  pr "/* This is passed to *_wrapper as the user_data pointer";
+  pr " * and freed in the free_user_data function below.\n";
+  pr " */\n";
+  pr "struct user_data {\n";
+  pr "  PyObject *fn;    /* Pointer to Python function. */\n";
+  pr "};\n";
+  pr "\n";
+  pr "static struct user_data *\n";
+  pr "alloc_user_data (void)\n";
+  pr "{\n";
+  pr "  struct user_data *data = calloc (1, sizeof *data);\n";
+  pr "  if (data == NULL) {\n";
+  pr "    PyErr_NoMemory ();\n";
+  pr "    return NULL;\n";
+  pr "  }\n";
+  pr "  return data;\n";
+  pr "}\n";
+  pr "\n";
+  pr "static void\n";
+  pr "free_user_data (void *user_data)\n";
+  pr "{\n";
+  pr "  struct user_data *data = user_data;\n";
+  pr "\n";
+  pr "  Py_DECREF (data->fn);\n";
+  pr "  free (data);\n";
+  pr "}\n";
+  pr "\n";
+
   List.iter print_python_closure_wrapper all_closures;
   List.iter (
     fun (name, fn) ->
-- 
2.22.0

---
 generator/generator | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/generator/generator b/generator/generator
index 8c055a9..1252bdb 100755
--- a/generator/generator
+++ b/generator/generator
@@ -4262,6 +4262,20 @@ let print_python_binding name { args; optargs; ret;
may_set_error }      | OFlags (n, _) -> pr "  %s_u32 = %s;\n" n n
   ) optargs;
 
+  (* If there is a BytesPersistIn/Out parameter then we need to
+   * increment the refcount and save the pointer into
+   * completion_callback.user_data so we can decrement the
+   * refcount on command completion.
+   *)
+  List.iter (
+    function
+    | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
+       pr "  /* Increment refcount since buffer may be saved by libnbd.
*/\n";
+       pr "  Py_INCREF (%s);\n" n;
+       pr "  completion_user_data->buf = %s;\n" n;
+    | _ -> ()
+  ) args;
+
   (* Call the underlying C function. *)
   pr "  ret = nbd_%s (h" name;
   List.iter (
@@ -4389,7 +4403,8 @@ let generate_python_methods_c ()    pr " * and freed
in the free_user_data function below.\n";
   pr " */\n";
   pr "struct user_data {\n";
-  pr "  PyObject *fn;    /* Pointer to Python function. */\n";
+  pr "  PyObject *fn;    /* Optional pointer to Python function.
*/\n";
+  pr "  PyObject *buf;   /* Optional pointer to persistent buffer.
*/\n";
   pr "};\n";
   pr "\n";
   pr "static struct user_data *\n";
@@ -4408,7 +4423,10 @@ let generate_python_methods_c ()    pr "{\n";
   pr "  struct user_data *data = user_data;\n";
   pr "\n";
-  pr "  Py_DECREF (data->fn);\n";
+  pr "  if (data->fn != NULL)\n";
+  pr "    Py_DECREF (data->fn);\n";
+  pr "  if (data->buf != NULL)\n";
+  pr "    Py_DECREF (data->buf);\n";
   pr "  free (data);\n";
   pr "}\n";
   pr "\n";
-- 
2.22.0

---
 python/t/590-aio-copy.py | 122 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/python/t/590-aio-copy.py b/python/t/590-aio-copy.py
new file mode 100644
index 0000000..129dde1
--- /dev/null
+++ b/python/t/590-aio-copy.py
@@ -0,0 +1,122 @@
+# libnbd Python bindings
+# Copyright (C) 2010-2019 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+import select
+import nbd
+
+disk_size = 512 * 1024 * 1024
+bs = 65536
+max_reads_in_flight = 16
+bytes_read = 0
+bytes_written = 0
+
+def asynch_copy (src, dst):
+    size = src.get_size ()
+
+    # This is our reading position in the source.
+    soff = 0
+
+    # This callback is called when any pread from the source
+    # has completed.
+    writes = []
+    def read_completed (buf, offset, error):
+        global bytes_read
+        bytes_read += buf.size ()
+        wr = (buf, offset)
+        writes.append (wr)
+        # By returning 1 here we auto-retire the pread command.
+        return 1
+
+    # This callback is called when any pwrite to the destination
+    # has completed.
+    def write_completed (buf, error):
+        global bytes_written
+        bytes_written += buf.size ()
+        # By returning 1 here we auto-retire the pwrite command.
+        return 1
+
+    # The main loop which runs until we have finished reading and
+    # there are no more commands in flight.
+    while soff < size or dst.aio_in_flight () > 0:
+        # If we're able to submit more reads from the source
+        # then do so now.
+        if soff < size and src.aio_in_flight () < max_reads_in_flight:
+            bufsize = min (bs, size - soff)
+            buf = nbd.Buffer (bufsize)
+            # NB: Python lambdas are BROKEN.
+            # https://stackoverflow.com/questions/2295290
+            src.aio_pread (buf, soff,
+                           lambda err, buf=buf, soff=soff:
+                           read_completed (buf, soff, err))
+            soff += bufsize
+
+        # If there are any write commands waiting to be issued
+        # to the destination, send them now.
+        for buf, offset in writes:
+            # See above link about broken Python lambdas.
+            dst.aio_pwrite (buf, offset,
+                            lambda err, buf=buf:
+                            write_completed (buf, err))
+        writes = []
+
+        poll = select.poll ()
+
+        sfd = src.aio_get_fd ()
+        dfd = dst.aio_get_fd ()
+
+        sevents = 0
+        devents = 0
+        if src.aio_get_direction () & nbd.AIO_DIRECTION_READ:
+            sevents += select.POLLIN
+        if src.aio_get_direction () & nbd.AIO_DIRECTION_WRITE:
+            sevents += select.POLLOUT
+        if dst.aio_get_direction () & nbd.AIO_DIRECTION_READ:
+            devents += select.POLLIN
+        if dst.aio_get_direction () & nbd.AIO_DIRECTION_WRITE:
+            devents += select.POLLOUT
+        poll.register (sfd, sevents)
+        poll.register (dfd, devents)
+        for (fd, revents) in poll.poll ():
+            # The direction of each handle can change since we
+            # slept in the select.
+            if fd == sfd and revents & select.POLLIN and \
+               src.aio_get_direction () & nbd.AIO_DIRECTION_READ:
+                src.aio_notify_read ()
+            elif fd == sfd and revents & select.POLLOUT and \
+                 src.aio_get_direction () & nbd.AIO_DIRECTION_WRITE:
+                src.aio_notify_write ()
+            elif fd == dfd and revents & select.POLLIN and \
+                 dst.aio_get_direction () & nbd.AIO_DIRECTION_READ:
+                dst.aio_notify_read ()
+            elif fd == dfd and revents & select.POLLOUT and \
+                 dst.aio_get_direction () & nbd.AIO_DIRECTION_WRITE:
+                dst.aio_notify_write ()
+
+src = nbd.NBD ()
+src.set_handle_name ("src")
+dst = nbd.NBD ()
+dst.set_handle_name ("dst")
+src.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-r",
+                      "pattern", "size=%d" % disk_size])
+dst.connect_command (["nbdkit", "-s",
"--exit-with-parent",
+                      "memory", "size=%d" % disk_size])
+asynch_copy (src, dst)
+
+print ("bytes read: %d written: %d size: %d" %
+       (bytes_read, bytes_written, disk_size))
+assert bytes_read == disk_size
+assert bytes_written == disk_size
-- 
2.22.0

---
 generator/generator | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/generator/generator b/generator/generator
index 1252bdb..13cb0b9 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3121,6 +3121,31 @@ let ()          failwithf "%s: first_version must be
1.x" name;
       if minor mod 2 <> 0 then
         failwithf "%s: first_version must refer to a stable release"
name
+  ) handle_calls;
+
+  (* Because of the way we use completion free callbacks to
+   * free persistent buffers in non-C languages, any function
+   * with a BytesPersistIn/Out parameter must have only one.
+   * And it must have an OClosure completion optarg.
+   *)
+  List.iter (
+    fun (name, { args; optargs }) ->
+      let is_persistent_buffer_arg = function
+        | BytesPersistIn _ | BytesPersistOut _ -> true
+        | _ -> false
+      and is_oclosure_completion = function
+        | OClosure { cbname = "completion" } -> true
+        | _ -> false
+      in
+      if List.exists is_persistent_buffer_arg args then (
+        let bpargs = List.filter is_persistent_buffer_arg args in
+        if List.length bpargs >= 2 then
+          failwithf "%s: multiple BytesPersistIn/Out params not
supported"
+            name;
+        if not (List.exists is_oclosure_completion optargs) then
+          failwithf "%s: functions with BytesPersistIn/Out arg must have
completion callback"
+            name
+      )
   ) handle_calls
 
 let generate_lib_libnbd_syms () -- 
2.22.0

On 8/15/19 4:56 AM, Richard W.M. Jones wrote:
> We have defined the concept of a "null callback" meaning one
where the
> .callback field = NULL.  The first two new macros just test this
> property, and the third one sets a callback to null.
> 
> This change is neutral refactoring.
> ---
>  generator/generator                 |  4 ++--
>  generator/states-reply-simple.c     |  2 +-
>  generator/states-reply-structured.c | 11 ++++++-----
>  generator/states-reply.c            |  2 +-
>  generator/states.c                  |  2 +-
>  lib/debug.c                         |  2 +-
>  lib/internal.h                      |  9 +++++++--
>  7 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/generator/generator b/generator/generator
> index ca97910..98c99e0 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -3576,7 +3576,7 @@ let generate_lib_api_c () >           let value =
match errcode with
>             | Some value -> value
>             | None -> assert false in
> -         pr "  if (%s_callback.callback == NULL) {\n" cbname;
> +         pr "  if (CALLBACK_IS_NULL (%s_callback)) {\n" cbname;
Slightly longer, but gives us a bit more freedom for future refactorings
(if everything goes through the macros rather than direct assignments,
then only the macro needs tweaking).

> +++ b/lib/internal.h
> @@ -273,6 +273,11 @@ struct command {
>    uint32_t error; /* Local errno value */
>  };
>  
> +/* Test if a callback is "null" or not, and set it to null. */
> +#define CALLBACK_IS_NULL(cb)     ((cb).callback == NULL)
> +#define CALLBACK_IS_NOT_NULL(cb) (! CALLBACK_IS_NULL ((cb)))
I don't know if I would have done this macro, but it doesn't hurt.
> +#define SET_CALLBACK_TO_NULL(cb) ((cb).callback = NULL)
> +
>  /* Call a callback. */
>  #define CALL_CALLBACK(cb, ...) \
>    (cb).callback ((cb).user_data, ##__VA_ARGS__)
> @@ -286,9 +291,9 @@ struct command {
>  #define FREE_CALLBACK(cb)                                               \
>    do {                                                                  \
>      nbd_completion_callback *_cb = (nbd_completion_callback *)&(cb);  
\
> -    if (_cb->callback != NULL && _cb->free != NULL)         
\
> +    if (CALLBACK_IS_NOT_NULL (cb) && _cb->free != NULL)        
\
>        _cb->free (_cb->user_data);                                   
\
> -    _cb->callback = NULL;                                              
\
> +    SET_CALLBACK_TO_NULL (cb);                                          \
>    } while (0)
>  
>  /* aio.c */
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 8/15/19 4:56 AM, Richard W.M. Jones wrote:
> Previously the .free function of a callback was not called if the
> .callback field was NULL, because the callback as a whole would be
> considered to be "null".
> 
> This change allows you to register callbacks where the .callback field
> is NULL, but the .free field is != NULL, meaning that the callback is
> freed after the last time it would have been used.
> 
> This is mainly convenient for language bindings where we sometimes
> want to register a free function to clean up a persistent buffer, but
> we don't need the associated completion callback to be actually
> called.
> ---
>  docs/libnbd.pod | 15 +++++++++++++++
>  lib/internal.h  | 18 +++++++++---------
>  2 files changed, 24 insertions(+), 9 deletions(-)
> 
> +++ b/lib/internal.h
> @@ -274,20 +274,20 @@ struct command {
>  };
>  
>  /* Test if a callback is "null" or not, and set it to null. */
> -#define CALLBACK_IS_NULL(cb)     ((cb).callback == NULL)
> +#define CALLBACK_IS_NULL(cb)     ((cb).callback == NULL &&
(cb).free == NULL)
Semantic change. In generator, you used CALLBACK_IS_NULL() for both
Closure and OClosure.  For OClosure, the new semantics are still
correct.  But for Closure, we now no longer return EFAULT when the
callback itself is missing but a .free was provided.  This changes
pread_structured and block_status to accept NULL for the callback; which
is probably not a good idea.

In short, there are some points in the code that care only whether
.callback is NULL, and others that care whether both pointers are NULL.

>  #define CALLBACK_IS_NOT_NULL(cb) (! CALLBACK_IS_NULL ((cb)))
> -#define SET_CALLBACK_TO_NULL(cb) ((cb).callback = NULL)
> +#define SET_CALLBACK_TO_NULL(cb) ((cb).callback = NULL, (cb).free = NULL)
>  
>  /* Call a callback. */
> -#define CALL_CALLBACK(cb, ...) \
> -  (cb).callback ((cb).user_data, ##__VA_ARGS__)
> +#define CALL_CALLBACK(cb, ...)                                          \
> +  ((cb).callback != NULL ? (cb).callback ((cb).user_data, ##__VA_ARGS__) :
0)
This one is nice, though.
>  
>  /* Free a callback. */
> -#define FREE_CALLBACK(cb)                                               \
> -  do {                                                                  \
> -    if (CALLBACK_IS_NOT_NULL (cb) && (cb).free != NULL)           
\
> -      (cb).free ((cb).user_data);                                       \
> -    SET_CALLBACK_TO_NULL (cb);                                          \
> +#define FREE_CALLBACK(cb)                               \
> +  do {                                                  \
> +    if ((cb).free != NULL)                              \
> +      (cb).free ((cb).user_data);                       \
> +    SET_CALLBACK_TO_NULL (cb);                          \
The SET_CALLBACK_TO_NULL usage is mostly dead code during command
retirement (as we are about to free() the struct containing cb in the
first place), but still vital when clearing the debug callback.  So this
part is also fine.
>    } while (0)
>  
>  /* aio.c */
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 8/15/19 4:56 AM, Richard W.M. Jones wrote:
> ---
>  generator/generator | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/generator/generator b/generator/generator
> index 1252bdb..13cb0b9 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -3121,6 +3121,31 @@ let () >          failwithf "%s:
first_version must be 1.x" name;
>        if minor mod 2 <> 0 then
>          failwithf "%s: first_version must refer to a stable
release" name
> +  ) handle_calls;
> +
> +  (* Because of the way we use completion free callbacks to
> +   * free persistent buffers in non-C languages, any function
> +   * with a BytesPersistIn/Out parameter must have only one.
> +   * And it must have an OClosure completion optarg.
> +   *)
I like it.  Other than the unintended semantic change in patch 4, I
think this series is ready to go.

We still had another potential API change to squeeze into 0.9.8:
Right now, the API is overloading 'command':
nbd_connect_command/nbd_kill_command
vs.
nbd_aio_command_completed/nbd_aio_peek_command_completed

Keeping nbd_connect_command may be okay (it takes a command line to
create a subprocess), but we may want to rename nbd_kill_command to
nbd_kill_child or similar, to make it obvious that it is NOT associated
with attempting an early abort of any synchronous nbd_pread or other
commands issued to the server.

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