Libguestfs - Aug 2019 - [PATCH libnbd] generator: Generate typedefs automatically for Closure arguments.

For example nbd_set_debug takes a callback function.  Previously this
was defined explicitly inside the function parameters.  This commit
defines a new public typedef:

  typedef int (*nbd_debug_callback) (unsigned valid_flag, void *user_data,
                                     const char *context, const char *msg);

and then uses the typedef like this:

  extern int nbd_set_debug_callback (struct nbd_handle *h,
                                     nbd_debug_callback debug_callback,
                                     void *debug_callback_user_data);

(Previously typedefs were available, but they were written by hand and
only used internally to the library.)

This change necessitates that we uniquely name all of our closures
across methods (the same-named closure is required to have the same
cbargs).

I took this opportunity to rename some, so especially completion
callbacks now have the type nbd_completion_callback.  The generator
also checks they are named uniquely.

This does not change the C API or ABI.
---
 generator/generator                 | 93 +++++++++++++++++++++++------
 generator/states-reply-simple.c     | 12 ++--
 generator/states-reply-structured.c | 40 ++++++-------
 generator/states-reply.c            |  8 +--
 generator/states.c                  |  8 +--
 lib/aio.c                           | 12 ++--
 lib/debug.c                         | 12 ++--
 lib/handle.c                        |  6 +-
 lib/internal.h                      | 20 ++-----
 lib/rw.c                            | 61 +++++++++++--------
 10 files changed, 165 insertions(+), 107 deletions(-)

diff --git a/generator/generator b/generator/generator
index 2a952cc..6f89792 100755
--- a/generator/generator
+++ b/generator/generator
@@ -924,7 +924,7 @@ Return the state of the debug flag on this handle.";
 
   "set_debug_callback", {
     default_call with
-    args = [ Closure { cbname="debug_fn";
+    args = [ Closure { cbname="debug";
                        cbargs=[String "context"; String
"msg"] } ];
     ret = RErr;
     shortdesc = "set the debug callback";
@@ -1731,7 +1731,7 @@ C<nbd_pread>.";
   "aio_pread_callback", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Closure { cbname="callback";
+             Closure { cbname="completion";
                        cbargs=[Mutable (Int "error")] };
              Flags "flags" ];
     ret = RInt64;
@@ -1778,7 +1778,7 @@ documented in C<nbd_pread_structured>.";
                                UInt64 "offset";
                                UInt "status";
                                Mutable (Int "error")] };
-             Closure { cbname="callback";
+             Closure { cbname="completion";
                        cbargs=[Mutable (Int "error")] };
              Flags "flags" ];
     ret = RInt64;
@@ -1814,7 +1814,7 @@ C<nbd_pwrite>.";
   "aio_pwrite_callback", {
     default_call with
     args = [ BytesPersistIn ("buf", "count"); UInt64
"offset";
-             Closure { cbname="callback";
+             Closure { cbname="completion";
                        cbargs=[Mutable (Int "error")] };
              Flags "flags" ];
     ret = RInt64;
@@ -1870,7 +1870,7 @@ Parameters behave as documented in
C<nbd_flush>.";
 
   "aio_flush_callback", {
     default_call with
-    args = [ Closure { cbname="callback";
+    args = [ Closure { cbname="completion";
                        cbargs=[Mutable (Int "error")] };
              Flags "flags" ];
     ret = RInt64;
@@ -1904,7 +1904,7 @@ Parameters behave as documented in
C<nbd_trim>.";
   "aio_trim_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Closure { cbname="callback";
+             Closure { cbname="completion";
                        cbargs=[Mutable (Int "error")] };
              Flags "flags" ];
     ret = RInt64;
@@ -1938,7 +1938,7 @@ Parameters behave as documented in
C<nbd_cache>.";
   "aio_cache_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Closure { cbname="callback";
+             Closure { cbname="completion";
                        cbargs=[Mutable (Int "error")] };
              Flags "flags" ];
     ret = RInt64;
@@ -1972,7 +1972,7 @@ Parameters behave as documented in
C<nbd_zero>.";
   "aio_zero_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Closure { cbname="callback";
+             Closure { cbname="completion";
                        cbargs=[Mutable (Int "error")] };
              Flags "flags" ];
     ret = RInt64;
@@ -2017,7 +2017,7 @@ Parameters behave as documented in
C<nbd_block_status>.";
                                ArrayAndLen (UInt32 "entries",
                                             "nr_entries");
                                Mutable (Int "error")] };
-             Closure { cbname="callback";
+             Closure { cbname="completion";
                        cbargs=[Mutable (Int "error")] };
              Flags "flags" ];
     ret = RInt64;
@@ -2363,6 +2363,25 @@ let rec group_by = function
 | (day, x) :: rest ->
    (day, [x]) :: group_by rest
 
+let uniq ?(cmp = Pervasives.compare) xs +  let rec loop acc = function
+    | [] -> acc
+    | [x] -> x :: acc
+    | x :: (y :: _ as xs) when cmp x y = 0 ->
+       loop acc xs
+    | x :: (y :: _ as xs) ->
+       loop (x :: acc) xs
+  in
+  List.rev (loop [] xs)
+
+(* This is present in OCaml 4.04, so we can remove it when
+ * we depend on OCaml >= 4.04.
+ *)
+let sort_uniq ?(cmp = Pervasives.compare) xs +  let xs = List.sort cmp xs in
+  let xs = uniq ~cmp xs in
+  xs
+
 let chan = ref Pervasives.stdout
 let pr fs = ksprintf (fun str -> output_string !chan str) fs
 
@@ -3100,6 +3119,30 @@ let ()                       name
   ) handle_calls;
 
+  (* Closures must be uniquely named across all calls. *)
+  let () +    let all_args +      List.flatten (List.map (fun (_, { args })
-> args) handle_calls) in
+    let h = Hashtbl.create 13 in
+    List.iter (
+      function
+      | Closure { cbname; cbargs } ->
+         (try
+            (* If we've already added this name to the hash, check
+             * closure args are identical.
+             *)
+            let other_cbargs = Hashtbl.find h cbname in
+            if cbargs <> other_cbargs then
+              failwithf "%s: Closure has different arguments across
methods"
+                cbname
+          with Not_found ->
+                (* Otherwise add it to the hash. *)
+                Hashtbl.add h cbname cbargs
+         )
+      | _ -> ()
+    ) all_args
+  in
+
   (* !may_set_error is incompatible with permitted_states != [] because
    * an incorrect state will result in set_error being called by the
    * generated wrapper.  It is also incompatible with RUint.
@@ -3170,7 +3213,8 @@ let rec name_of_arg = function
 | BytesOut (n, len) -> [n; len]
 | BytesPersistIn (n, len) -> [n; len]
 | BytesPersistOut (n, len) -> [n; len]
-| Closure { cbname } -> [cbname; sprintf "%s_user_data" cbname ]
+| Closure { cbname } ->
+   [ sprintf "%s_callback" cbname; sprintf "%s_user_data"
cbname ]
 | Flags n -> [n]
 | Int n -> [n]
 | Int64 n -> [n]
@@ -3232,12 +3276,8 @@ let rec print_arg_list ?(handle = false) ?(valid_flag =
false)
          if types then pr "size_t ";
          pr "%s" len
       | Closure { cbname; cbargs } ->
-         if types then (
-           pr "int (*%s) " cbname;
-           print_arg_list ~valid_flag:true ~user_data:true cbargs;
-         )
-         else
-           pr "%s" cbname;
+         if types then pr "nbd_%s_callback " cbname;
+         pr "%s_callback" cbname;
          pr ", ";
          if types then pr "void *";
          pr "%s_user_data" cbname
@@ -3297,6 +3337,24 @@ let print_extern name args ret    print_call name args
ret;
   pr ";\n"
 
+(* Callback typedefs in <libnbd.h> *)
+let print_closure_typedefs () +  let all_cls +    List.map (
+      fun (_, { args }) ->
+        filter_map (function Closure cl -> Some cl | _ -> None) args
+    ) handle_calls in
+  let all_cls = List.flatten all_cls in
+  let cmp { cbname = n1 } { cbname = n2 } = compare n1 n2 in
+  let unique_cls = sort_uniq ~cmp all_cls in
+  List.iter (
+    fun { cbname; cbargs } ->
+      pr "typedef int (*nbd_%s_callback) " cbname;
+      print_arg_list ~valid_flag:true ~user_data:true cbargs;
+      pr ";\n";
+  ) unique_cls;
+  pr "\n"
+
 let print_extern_and_define name args ret    let name_upper =
String.uppercase_ascii name in
   print_extern name args ret;
@@ -3355,6 +3413,7 @@ let generate_include_libnbd_h ()    pr "extern int
nbd_get_errno (void);\n";
   pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n";
   pr "\n";
+  print_closure_typedefs ();
   List.iter (
     fun (name, { args; ret }) -> print_extern_and_define name args ret
   ) handle_calls;
@@ -4845,7 +4904,7 @@ let print_ocaml_binding (name, { args; ret })         pr
"  if (%s_user_data == NULL) caml_raise_out_of_memory ();\n" cbname;
        pr "  caml_register_generational_global_root
(%s_user_data);\n" cbname;
        pr "  *%s_user_data = %sv;\n" cbname cbname;
-       pr "  const void *%s = %s_%s_wrapper;\n" cbname name cbname
+       pr "  const void *%s_callback = %s_%s_wrapper;\n" cbname name
cbname
     | OCamlArg (Flags _) -> assert false (* see above *)
     | OCamlArg (Int n) ->
        pr "  int %s = Int_val (%sv);\n" n n
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index e0fd71d..9b249ab 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -60,16 +60,16 @@
   case 0:
     /* guaranteed by START */
     assert (cmd);
-    if (cmd->cb.fn.read) {
+    if (cmd->cb.fn.chunk) {
       int error = 0;
 
       assert (cmd->error == 0);
-      if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
-                           cmd->cb.fn_user_data,
-                           cmd->data, cmd->count,
-                           cmd->offset, LIBNBD_READ_DATA, &error) == -1)
+      if (cmd->cb.fn.chunk (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
+                            cmd->cb.fn_user_data,
+                            cmd->data, cmd->count,
+                            cmd->offset, LIBNBD_READ_DATA, &error) ==
-1)
         cmd->error = error ? error : EPROTO;
-      cmd->cb.fn.read = NULL; /* because we've freed it */
+      cmd->cb.fn.chunk = NULL; /* because we've freed it */
     }
 
     SET_NEXT_STATE (%^FINISH_COMMAND);
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index ff5b727..cdd9f10 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -304,7 +304,7 @@ valid_flags (struct nbd_handle *h)
                    offset, cmd->offset, cmd->count);
         return 0;
       }
-      if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read) {
+      if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) {
         int scratch = error;
         unsigned valid = valid_flags (h);
 
@@ -312,13 +312,13 @@ valid_flags (struct nbd_handle *h)
          * current error rather than any earlier one. If the callback fails
          * without setting errno, then use the server's error below.
          */
-        if (cmd->cb.fn.read (valid, cmd->cb.fn_user_data,
-                             cmd->data + (offset - cmd->offset),
-                             0, offset, LIBNBD_READ_ERROR, &scratch) == -1)
+        if (cmd->cb.fn.chunk (valid, 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)
-          cmd->cb.fn.read = NULL; /* because we've freed it */
+          cmd->cb.fn.chunk = NULL; /* because we've freed it */
       }
     }
 
@@ -398,18 +398,18 @@ valid_flags (struct nbd_handle *h)
     offset = be64toh (h->sbuf.sr.payload.offset_data.offset);
 
     assert (cmd); /* guaranteed by CHECK */
-    if (cmd->cb.fn.read) {
+    if (cmd->cb.fn.chunk) {
       int error = cmd->error;
       unsigned valid = valid_flags (h);
 
-      if (cmd->cb.fn.read (valid, cmd->cb.fn_user_data,
-                           cmd->data + (offset - cmd->offset),
-                           length - sizeof offset, offset,
+      if (cmd->cb.fn.chunk (valid, 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)
-        cmd->cb.fn.read = NULL; /* because we've freed it */
+        cmd->cb.fn.chunk = NULL; /* because we've freed it */
     }
 
     SET_NEXT_STATE (%FINISH);
@@ -463,18 +463,18 @@ valid_flags (struct nbd_handle *h)
      * them as an extension, and this works even when length == 0.
      */
     memset (cmd->data + offset, 0, length);
-    if (cmd->cb.fn.read) {
+    if (cmd->cb.fn.chunk) {
       int error = cmd->error;
       unsigned valid = valid_flags (h);
 
-      if (cmd->cb.fn.read (valid, cmd->cb.fn_user_data,
-                           cmd->data + offset, length,
-                           cmd->offset + offset,
-                           LIBNBD_READ_HOLE, &error) == -1)
+      if (cmd->cb.fn.chunk (valid, 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)
-        cmd->cb.fn.read = NULL; /* because we've freed it */
+        cmd->cb.fn.chunk = NULL; /* because we've freed it */
     }
 
     SET_NEXT_STATE(%FINISH);
@@ -548,10 +548,10 @@ valid_flags (struct nbd_handle *h)
     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.read)
-      cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
-                       NULL, 0, 0, 0, NULL);
-    cmd->cb.fn.read = 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);
+    cmd->cb.fn.chunk = NULL;
     SET_NEXT_STATE (%^FINISH_COMMAND);
   }
   else {
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 078d67f..09adfed 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -168,14 +168,14 @@ save_reply_state (struct nbd_handle *h)
   retire = cmd->type == NBD_CMD_DISC;
 
   /* Notify the user */
-  if (cmd->cb.callback) {
+  if (cmd->cb.completion) {
     int error = cmd->error;
     int r;
 
     assert (cmd->type != NBD_CMD_DISC);
-    r = cmd->cb.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
-                          cmd->cb.user_data, &error);
-    cmd->cb.callback = NULL; /* because we've freed it */
+    r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
+                            cmd->cb.user_data, &error);
+    cmd->cb.completion = NULL; /* because we've freed it */
     switch (r) {
     case -1:
       if (error)
diff --git a/generator/states.c b/generator/states.c
index 654e4c8..9ed57ae 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -121,14 +121,14 @@ void abort_commands (struct nbd_handle *h,
     bool retire = cmd->type == NBD_CMD_DISC;
 
     next = cmd->next;
-    if (cmd->cb.callback) {
+    if (cmd->cb.completion) {
       int error = cmd->error ? cmd->error : ENOTCONN;
       int r;
 
       assert (cmd->type != NBD_CMD_DISC);
-      r = cmd->cb.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
-                            cmd->cb.user_data, &error);
-      cmd->cb.callback = NULL; /* because we've freed it */
+      r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
+                              cmd->cb.user_data, &error);
+      cmd->cb.completion = NULL; /* because we've freed it */
       switch (r) {
       case -1:
         if (error)
diff --git a/lib/aio.c b/lib/aio.c
index 1c11dbd..c141de6 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -35,12 +35,12 @@ nbd_internal_retire_and_free_command (struct command *cmd)
   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.read)
-    cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
-                     NULL, 0, 0, 0, NULL);
-  if (cmd->cb.callback)
-    cmd->cb.callback (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
-                      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 (cmd);
 }
diff --git a/lib/debug.c b/lib/debug.c
index 7784bd9..f4b374d 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -40,12 +40,12 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
 
 int
 nbd_unlocked_set_debug_callback (struct nbd_handle *h,
-                                 debug_fn debug_fn, void *data)
+                                 nbd_debug_callback debug_callback, void *data)
 {
-  if (h->debug_fn)
+  if (h->debug_callback)
     /* ignore return value */
-    h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
-  h->debug_fn = debug_fn;
+    h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
+  h->debug_callback = debug_callback;
   h->debug_data = data;
   return 0;
 }
@@ -76,9 +76,9 @@ nbd_internal_debug (struct nbd_handle *h, const char *fs, ...)
   if (r == -1)
     goto out;
 
-  if (h->debug_fn)
+  if (h->debug_callback)
     /* ignore return value */
-    h->debug_fn (LIBNBD_CALLBACK_VALID, h->debug_data, context, msg);
+    h->debug_callback (LIBNBD_CALLBACK_VALID, h->debug_data, context,
msg);
   else
     fprintf (stderr, "libnbd: debug: %s: %s\n", context ? :
"unknown", msg);
  out:
diff --git a/lib/handle.c b/lib/handle.c
index a9ade3d..1360079 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -105,9 +105,9 @@ nbd_close (struct nbd_handle *h)
     return;
 
   /* Free user callbacks first. */
-  if (h->debug_fn)
-    h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
-  h->debug_fn = NULL;
+  if (h->debug_callback)
+    h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
+  h->debug_callback = NULL;
 
   free (h->bs_entries);
   for (m = h->meta_contexts; m != NULL; m = m_next) {
diff --git a/lib/internal.h b/lib/internal.h
index 90ce6aa..d85fb4b 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -47,7 +47,6 @@
 struct meta_context;
 struct socket;
 struct command;
-typedef int (*debug_fn) (unsigned, void *, const char *, const char *);
 
 struct nbd_handle {
   /* Lock protecting concurrent access to the handle. */
@@ -80,7 +79,7 @@ struct nbd_handle {
 
   /* For debugging. */
   bool debug;
-  debug_fn debug_fn;
+  nbd_debug_callback debug_callback;
   void *debug_data;
 
   /* State machine.
@@ -248,23 +247,14 @@ struct socket {
   const struct socket_ops *ops;
 };
 
-typedef int (*extent_fn) (unsigned valid_flag, void *user_data,
-                          const char *metacontext, uint64_t offset,
-                          uint32_t *entries, size_t nr_entries, int *error);
-typedef int (*read_fn) (unsigned valid_flag, void *user_data,
-                        const void *buf, size_t count,
-                        uint64_t offset, unsigned status, int *error);
-typedef int (*callback_fn) (unsigned valid_flag, void *user_data,
-                            int *error);
-
 struct command_cb {
   union {
-    extent_fn extent;
-    read_fn read;
+    nbd_extent_callback extent;
+    nbd_chunk_callback chunk;
   } fn;
   void *fn_user_data; /* associated with one of the fn callbacks above */
-  callback_fn callback;
-  void *user_data; /* associated with the callback function */
+  nbd_completion_callback completion;
+  void *user_data; /* associated with the completion callback */
 };
 
 struct command {
diff --git a/lib/rw.c b/lib/rw.c
index 382cfb9..51ee691 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -60,12 +60,13 @@ nbd_unlocked_pread (struct nbd_handle *h, void *buf,
 int
 nbd_unlocked_pread_structured (struct nbd_handle *h, void *buf,
                                size_t count, uint64_t offset,
-                               read_fn read, void *user_data, uint32_t flags)
+                               nbd_chunk_callback chunk, void *user_data,
+                               uint32_t flags)
 {
   int64_t cookie;
 
   cookie = nbd_unlocked_aio_pread_structured (h, buf, count, offset,
-                                              read, user_data, flags);
+                                              chunk, user_data, flags);
   if (cookie == -1)
     return -1;
 
@@ -145,7 +146,8 @@ nbd_unlocked_zero (struct nbd_handle *h,
 int
 nbd_unlocked_block_status (struct nbd_handle *h,
                            uint64_t count, uint64_t offset,
-                           extent_fn extent, void *user_data, uint32_t flags)
+                           nbd_extent_callback extent, void *user_data,
+                           uint32_t flags)
 {
   int64_t cookie;
 
@@ -255,10 +257,11 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
 int64_t
 nbd_unlocked_aio_pread_callback (struct nbd_handle *h, void *buf,
                                  size_t count, uint64_t offset,
-                                 callback_fn callback, void *user_data,
+                                 nbd_completion_callback completion,
+                                 void *user_data,
                                  uint32_t flags)
 {
-  struct command_cb cb = { .callback = callback, .user_data = user_data, };
+  struct command_cb cb = { .completion = completion, .user_data = user_data, };
 
   /* We could silently accept flag DF, but it really only makes sense
    * with callbacks, because otherwise there is no observable change
@@ -276,11 +279,11 @@ nbd_unlocked_aio_pread_callback (struct nbd_handle *h,
void *buf,
 int64_t
 nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf,
                                    size_t count, uint64_t offset,
-                                   read_fn read, void *user_data,
+                                   nbd_chunk_callback chunk, void *user_data,
                                    uint32_t flags)
 {
   return nbd_unlocked_aio_pread_structured_callback (h, buf, count, offset,
-                                                     read, user_data,
+                                                     chunk, user_data,
                                                      NULL, NULL,
                                                      flags);
 }
@@ -288,15 +291,15 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h,
void *buf,
 int64_t
 nbd_unlocked_aio_pread_structured_callback (struct nbd_handle *h, void *buf,
                                             size_t count, uint64_t offset,
-                                            read_fn read,
+                                            nbd_chunk_callback chunk,
                                             void *read_user_data,
-                                            callback_fn callback,
+                                            nbd_completion_callback completion,
                                             void *callback_user_data,
                                             uint32_t flags)
 {
-  struct command_cb cb = { .fn.read = read,
+  struct command_cb cb = { .fn.chunk = chunk,
                            .fn_user_data = read_user_data,
-                           .callback = callback,
+                           .completion = completion,
                            .user_data = callback_user_data, };
 
   if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
@@ -326,10 +329,11 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void
*buf,
 int64_t
 nbd_unlocked_aio_pwrite_callback (struct nbd_handle *h, const void *buf,
                                   size_t count, uint64_t offset,
-                                  callback_fn callback, void *user_data,
+                                  nbd_completion_callback completion,
+                                  void *user_data,
                                   uint32_t flags)
 {
-  struct command_cb cb = { .callback = callback, .user_data = user_data, };
+  struct command_cb cb = { .completion = completion, .user_data = user_data, };
 
   if (nbd_unlocked_read_only (h) == 1) {
     set_error (EINVAL, "server does not support write operations");
@@ -358,10 +362,12 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, uint32_t
flags)
 }
 
 int64_t
-nbd_unlocked_aio_flush_callback (struct nbd_handle *h, callback_fn callback,
-                                 void *user_data, uint32_t flags)
+nbd_unlocked_aio_flush_callback (struct nbd_handle *h,
+                                 nbd_completion_callback completion,
+                                 void *user_data,
+                                 uint32_t flags)
 {
-  struct command_cb cb = { .callback = callback, .user_data = user_data, };
+  struct command_cb cb = { .completion = completion, .user_data = user_data, };
 
   if (nbd_unlocked_can_flush (h) != 1) {
     set_error (EINVAL, "server does not support flush operations");
@@ -388,10 +394,11 @@ nbd_unlocked_aio_trim (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_trim_callback (struct nbd_handle *h,
                                 uint64_t count, uint64_t offset,
-                                callback_fn callback, void *user_data,
+                                nbd_completion_callback completion,
+                                void *user_data,
                                 uint32_t flags)
 {
-  struct command_cb cb = { .callback = callback, .user_data = user_data, };
+  struct command_cb cb = { .completion = completion, .user_data = user_data, };
 
   if (nbd_unlocked_read_only (h) == 1) {
     set_error (EINVAL, "server does not support write operations");
@@ -428,10 +435,11 @@ nbd_unlocked_aio_cache (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_cache_callback (struct nbd_handle *h,
                                  uint64_t count, uint64_t offset,
-                                 callback_fn callback, void *user_data,
+                                 nbd_completion_callback completion,
+                                 void *user_data,
                                  uint32_t flags)
 {
-  struct command_cb cb = { .callback = callback, .user_data = user_data, };
+  struct command_cb cb = { .completion = completion, .user_data = user_data, };
 
   /* Actually according to the NBD protocol document, servers do exist
    * that support NBD_CMD_CACHE but don't advertise the
@@ -462,10 +470,11 @@ nbd_unlocked_aio_zero (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_zero_callback (struct nbd_handle *h,
                                 uint64_t count, uint64_t offset,
-                                callback_fn callback, void *user_data,
+                                nbd_completion_callback completion,
+                                void *user_data,
                                 uint32_t flags)
 {
-  struct command_cb cb = { .callback = callback, .user_data = user_data, };
+  struct command_cb cb = { .completion = completion, .user_data = user_data, };
 
   if (nbd_unlocked_read_only (h) == 1) {
     set_error (EINVAL, "server does not support write operations");
@@ -495,7 +504,7 @@ nbd_unlocked_aio_zero_callback (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_block_status (struct nbd_handle *h,
                                uint64_t count, uint64_t offset,
-                               extent_fn extent, void *user_data,
+                               nbd_extent_callback extent, void *user_data,
                                uint32_t flags)
 {
   return nbd_unlocked_aio_block_status_callback (h, count, offset,
@@ -507,15 +516,15 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_block_status_callback (struct nbd_handle *h,
                                         uint64_t count, uint64_t offset,
-                                        extent_fn extent,
+                                        nbd_extent_callback extent,
                                         void *extent_user_data,
-                                        callback_fn callback,
+                                        nbd_completion_callback completion,
                                         void *callback_user_data,
                                         uint32_t flags)
 {
   struct command_cb cb = { .fn.extent = extent,
                            .fn_user_data = extent_user_data,
-                           .callback = callback,
+                           .completion = completion,
                            .user_data = callback_user_data };
 
   if (!h->structured_replies) {
-- 
2.22.0

On 8/3/19 7:42 AM, Richard W.M. Jones wrote:
> For example nbd_set_debug takes a callback function.  Previously this
> was defined explicitly inside the function parameters.  This commit
> defines a new public typedef:
> 
>   typedef int (*nbd_debug_callback) (unsigned valid_flag, void *user_data,
>                                      const char *context, const char *msg);
> 
> and then uses the typedef like this:
> 
>   extern int nbd_set_debug_callback (struct nbd_handle *h,
>                                      nbd_debug_callback debug_callback,
>                                      void *debug_callback_user_data);
> 
> (Previously typedefs were available, but they were written by hand and
> only used internally to the library.)
Yes, that aids legibility.
> 
> This change necessitates that we uniquely name all of our closures
> across methods (the same-named closure is required to have the same
> cbargs).
> 
> I took this opportunity to rename some, so especially completion
> callbacks now have the type nbd_completion_callback.  The generator
> also checks they are named uniquely.
> 
> This does not change the C API or ABI.
Concur.

The patch looks good to me.

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