Libguestfs - Aug 2019 - [PATCH libnbd v2 2/3] lib: Make all completion callbacks into OClosures.

v1 was here:
https://www.redhat.com/archives/libguestfs/2019-August/msg00168.html

I pushed uncontroversial patches 1-4

v2:

- The implementation of OClosure (new patch 1) in Python is fixed.

- Patch 2 (old patch 5) is unchanged.

- I added a new API for removing debug callbacks.  I think this
  approach has some advantages over using OClosure.

- I didn't yet do any work on changing the aio_*_callback
  functions to aio_*.

Rich.

An optional Closure parameter, but otherwise works the same way as
Closure.
---
 generator/generator | 57 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/generator/generator b/generator/generator
index 8cf95b6..8f15786 100755
--- a/generator/generator
+++ b/generator/generator
@@ -867,6 +867,7 @@ and arg  | UInt32 of string         (* 32 bit unsigned int
*)
 | UInt64 of string         (* 64 bit unsigned int *)
 and optarg +| OClosure of closure      (* optional closure *)
 | OFlags of string * flags (* optional flags, uint32_t in C *)
 and ret  | RBool                    (* return a boolean, or error *)
@@ -3184,16 +3185,6 @@ end = struct
 
 (* Check the API definition. *)
 let () -  (* Currently optargs can only be [] or [OFlags].  This condition
-   * will be relaxed later when we support more optional arguments.
-   *)
-  List.iter (
-    function
-    | _, { optargs = [] } | _, { optargs = [OFlags _] } -> ()
-    | (name, _) ->
-       failwithf "%s: optargs can only be empty list or [OFlags]"
name
-  ) handle_calls;
-
   (* Check functions using may_set_error. *)
   List.iter (
     function
@@ -3226,7 +3217,8 @@ let ()      | name, { optargs; may_set_error = false }
          when List.exists
                 (function
-                 | OFlags _ -> true) optargs ->
+                 | OFlags _ -> true
+                 | _ -> false) optargs ->
        failwithf "%s: if optargs contains an OFlags parameter,
may_set_error must be false" name
 
     | _ -> ()
@@ -3388,6 +3380,12 @@ let rec print_arg_list ?(handle = false) ?(types = true)
args optargs        if !comma then pr ", ";
       comma := true;
       match optarg with
+      | OClosure { cbname; cbargs } ->
+         if types then pr "nbd_%s_callback " cbname;
+         pr "%s_callback" cbname;
+         pr ", ";
+         if types then pr "void *";
+         pr "%s_user_data" cbname
       | OFlags (n, _) ->
          if types then pr "uint32_t ";
          pr "%s" n
@@ -3718,6 +3716,7 @@ let generate_lib_api_c ()      ) args;
     List.iter (
       function
+      | OClosure _ -> ()
       | OFlags (n, flags) ->
          print_flags_check n flags
     ) optargs;
@@ -3767,6 +3766,7 @@ let generate_lib_api_c ()      ) args;
     List.iter (
       function
+      | OClosure { cbname } -> pr " %s=%%s" cbname
       | OFlags (n, _) -> pr " %s=0x%%x" n
     ) optargs;
     pr "\"";
@@ -3789,6 +3789,7 @@ let generate_lib_api_c ()      ) args;
     List.iter (
       function
+      | OClosure { cbname } -> pr ", %s_callback ?
\"<fun>\" : \"NULL\"" cbname
       | OFlags (n, _) -> pr ", %s" n
     ) optargs;
     pr ");\n"
@@ -4297,6 +4298,8 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
+    | OClosure { cbname } ->
+       pr "  PyObject *%s_user_data;\n" cbname
     | OFlags (n, _) ->
        pr "  uint32_t %s_u32;\n" n;
        pr "  unsigned int %s; /* really uint32_t */\n" n
@@ -4327,6 +4330,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
+    | OClosure _ -> pr " \"O\""
     | OFlags _ -> pr " \"I\""
   ) optargs;
   pr "\n";
@@ -4353,6 +4357,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
+    | OClosure { cbname } -> pr ", &%s_user_data" cbname
     | OFlags (n, _) -> pr ", &%s" n
   ) optargs;
   pr "))\n";
@@ -4394,6 +4399,16 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
+    | OClosure { cbname } ->
+       pr "  if (%s_user_data != 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 "      PyErr_SetString (PyExc_TypeError,\n";
+       pr "                       \"callback parameter %s is not
callable\");\n" cbname;
+       pr "      return NULL;\n";
+       pr "    }\n";
+       pr "  }\n"
     | OFlags (n, _) -> pr "  %s_u32 = %s;\n" n n
   ) optargs;
 
@@ -4423,6 +4438,9 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
+    | OClosure { cbname } ->
+       pr ", %s_user_data ? %s_wrapper : NULL" cbname cbname;
+       pr ", %s_user_data" cbname
     | OFlags (n, _) -> pr ", %s_u32" n
   ) optargs;
   pr ");\n";
@@ -4679,6 +4697,7 @@ class NBD (object):
       let optargs          List.map (
           function
+          | OClosure { cbname } -> cbname, Some "None", None
           | OFlags (n, _) -> n, Some "0", None
         ) optargs in
       let args = args @ optargs in
@@ -4767,6 +4786,8 @@ and ocaml_ret_to_string = function
   | RUInt -> "int"
 
 and ocaml_optarg_to_string = function
+  | OClosure { cbname; cbargs } ->
+     sprintf "?%s:(%s)" cbname (ocaml_closuredecl_to_string cbargs)
   | OFlags (n, { flag_prefix }) -> sprintf "?%s:%s.t list" n
flag_prefix
 
 and ocaml_closuredecl_to_string cbargs @@ -4805,6 +4826,7 @@ let
ocaml_name_of_arg = function
   | UInt64 n -> n
 
 let ocaml_name_of_optarg = function
+  | OClosure { cbname } -> cbname
   | OFlags (n, _) -> n
 
 let num_params args optargs @@ -5213,6 +5235,19 @@ let print_ocaml_binding
(name, { args; optargs; ret })
   List.iter (
     function
+    | OClosure { cbname } ->
+       pr "  const void *%s_callback = NULL;\n" cbname;
+       pr "  value *%s_user_data = NULL;\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 "    %s_user_data = malloc (sizeof (value));\n" cbname;
+       pr "    if (%s_user_data == NULL) caml_raise_out_of_memory
();\n" cbname;
+       pr "    *%s_user_data = Field (%sv, 0);\n" cbname cbname;
+       pr "    caml_register_generational_global_root
(%s_user_data);\n" cbname;
+       pr "    %s_callback = %s_wrapper;\n" cbname cbname;
+       pr "  }\n";
     | OFlags (n, { flag_prefix }) ->
        pr "  uint32_t %s;\n" n;
        pr "  if (%sv != Val_int (0)) /* Some [ list of %s.t ] */\n"
-- 
2.22.0

This doesn't change the C API, except that it is now permitted to pass
NULL, NULL for the completion callback.  For the Python and OCaml
APIs the parameter changes to be an optional parameter.
---
 generator/generator                           | 36 +++++++++----------
 ocaml/examples/asynch_copy.ml                 |  5 +--
 .../test_505_aio_pread_structured_callback.ml | 12 ++++---
 ocaml/tests/test_590_aio_copy.ml              |  5 +--
 4 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/generator/generator b/generator/generator
index 8f15786..6ccfc5f 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1836,9 +1836,8 @@ C<nbd_pread>.";
 
   "aio_pread_callback", {
     default_call with
-    args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Closure completion_closure ];
-    optargs = [ OFlags ("flags", cmd_flags) ];
+    args = [ BytesPersistOut ("buf", "count"); UInt64
"offset" ];
+    optargs = [ OClosure completion_closure; OFlags ("flags",
cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
     shortdesc = "read from the NBD server, with callback on
completion";
@@ -1874,9 +1873,8 @@ documented in C<nbd_pread_structured>.";
   "aio_pread_structured_callback", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Closure chunk_closure;
-             Closure completion_closure ];
-    optargs = [ OFlags ("flags", cmd_flags) ];
+             Closure chunk_closure ];
+    optargs = [ OClosure completion_closure; OFlags ("flags",
cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
     shortdesc = "read from the NBD server, with callback on
completion";
@@ -1910,9 +1908,8 @@ C<nbd_pwrite>.";
 
   "aio_pwrite_callback", {
     default_call with
-    args = [ BytesPersistIn ("buf", "count"); UInt64
"offset";
-             Closure completion_closure ];
-    optargs = [ OFlags ("flags", cmd_flags) ];
+    args = [ BytesPersistIn ("buf", "count"); UInt64
"offset" ];
+    optargs = [ OClosure completion_closure; OFlags ("flags",
cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
     shortdesc = "write to the NBD server, with callback on
completion";
@@ -1966,8 +1963,8 @@ Parameters behave as documented in
C<nbd_flush>.";
 
   "aio_flush_callback", {
     default_call with
-    args = [ Closure completion_closure ];
-    optargs = [ OFlags ("flags", cmd_flags) ];
+    args = [];
+    optargs = [ OClosure completion_closure; OFlags ("flags",
cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
     shortdesc = "send flush command to the NBD server, with callback on
completion";
@@ -1999,8 +1996,8 @@ Parameters behave as documented in
C<nbd_trim>.";
 
   "aio_trim_callback", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; Closure
completion_closure ];
-    optargs = [ OFlags ("flags", cmd_flags) ];
+    args = [ UInt64 "count"; UInt64 "offset" ];
+    optargs = [ OClosure completion_closure; OFlags ("flags",
cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
     shortdesc = "send trim command to the NBD server, with callback on
completion";
@@ -2032,8 +2029,8 @@ Parameters behave as documented in
C<nbd_cache>.";
 
   "aio_cache_callback", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; Closure
completion_closure ];
-    optargs = [ OFlags ("flags", cmd_flags) ];
+    args = [ UInt64 "count"; UInt64 "offset" ];
+    optargs = [ OClosure completion_closure; OFlags ("flags",
cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
     shortdesc = "send cache (prefetch) command to the NBD server, with
callback on completion";
@@ -2065,8 +2062,8 @@ Parameters behave as documented in
C<nbd_zero>.";
 
   "aio_zero_callback", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; Closure
completion_closure ];
-    optargs = [ OFlags ("flags", cmd_flags) ];
+    args = [ UInt64 "count"; UInt64 "offset" ];
+    optargs = [ OClosure completion_closure; OFlags ("flags",
cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
     shortdesc = "send write zeroes command to the NBD server, with
callback on completion";
@@ -2098,9 +2095,8 @@ Parameters behave as documented in
C<nbd_block_status>.";
 
   "aio_block_status_callback", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset";
-             Closure extent_closure; Closure completion_closure ];
-    optargs = [ OFlags ("flags", cmd_flags) ];
+    args = [ UInt64 "count"; UInt64 "offset"; Closure
extent_closure ];
+    optargs = [ OClosure completion_closure; OFlags ("flags",
cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
     shortdesc = "send block status command to the NBD server, with
callback on completion";
diff --git a/ocaml/examples/asynch_copy.ml b/ocaml/examples/asynch_copy.ml
index 7dbe976..5aa6e60 100644
--- a/ocaml/examples/asynch_copy.ml
+++ b/ocaml/examples/asynch_copy.ml
@@ -49,7 +49,7 @@ let asynch_copy src dst        let bs = min bs (size -^ !soff)
in
       let buf = NBD.Buffer.alloc (Int64.to_int bs) in
       ignore (NBD.aio_pread_callback src buf !soff
-                (read_completed buf !soff));
+                ~completion:(read_completed buf !soff));
       soff := !soff +^ bs
     );
 
@@ -59,7 +59,8 @@ let asynch_copy src dst      List.iter (
       fun (buf, offset) ->
         (* Note the size of the write is implicitly stored in buf. *)
-        ignore (NBD.aio_pwrite_callback dst buf offset (write_completed buf))
+        ignore (NBD.aio_pwrite_callback dst buf offset
+                  ~completion:(write_completed buf))
     ) !writes;
     writes := [];
 
diff --git a/ocaml/tests/test_505_aio_pread_structured_callback.ml
b/ocaml/tests/test_505_aio_pread_structured_callback.ml
index 9b9ac2c..dc0d557 100644
--- a/ocaml/tests/test_505_aio_pread_structured_callback.ml
+++ b/ocaml/tests/test_505_aio_pread_structured_callback.ml
@@ -60,7 +60,8 @@ let ()  
   (* First try: succeed in both callbacks *)
   let buf = NBD.Buffer.alloc 512 in
-  let cookie = NBD.aio_pread_structured_callback nbd buf 0_L (chunk 42)
(callback 42) in
+  let cookie = NBD.aio_pread_structured_callback nbd buf 0_L (chunk 42)
+                 ~completion:(callback 42) in
   while not (NBD.aio_command_completed nbd cookie) do
     ignore (NBD.poll nbd (-1))
   done;
@@ -71,7 +72,8 @@ let ()  
   (* Second try: fail only during callback *)
   let buf = NBD.Buffer.alloc 512 in
-  let cookie = NBD.aio_pread_structured_callback nbd buf 0_L (chunk 42)
(callback 43) in
+  let cookie = NBD.aio_pread_structured_callback nbd buf 0_L (chunk 42)
+                 ~completion:(callback 43) in
   try
     while not (NBD.aio_command_completed nbd cookie) do
       ignore (NBD.poll nbd (-1))
@@ -84,7 +86,8 @@ let ()  
   (* Third try: fail during both *)
   let buf = NBD.Buffer.alloc 512 in
-  let cookie = NBD.aio_pread_structured_callback nbd buf 0_L (chunk 43)
(callback 44) in
+  let cookie = NBD.aio_pread_structured_callback nbd buf 0_L (chunk 43)
+                 ~completion:(callback 44) in
   try
     while not (NBD.aio_command_completed nbd cookie) do
       ignore (NBD.poll nbd (-1))
@@ -97,7 +100,8 @@ let ()  
   (* Fourth try: fail only during chunk *)
   let buf = NBD.Buffer.alloc 512 in
-  let cookie = NBD.aio_pread_structured_callback nbd buf 0_L (chunk 43)
(callback 42) in
+  let cookie = NBD.aio_pread_structured_callback nbd buf 0_L (chunk 43)
+                 ~completion:(callback 42) in
   try
     while not (NBD.aio_command_completed nbd cookie) do
       ignore (NBD.poll nbd (-1))
diff --git a/ocaml/tests/test_590_aio_copy.ml b/ocaml/tests/test_590_aio_copy.ml
index 290ca93..18ce389 100644
--- a/ocaml/tests/test_590_aio_copy.ml
+++ b/ocaml/tests/test_590_aio_copy.ml
@@ -72,7 +72,7 @@ let asynch_copy src dst        let bs = min bs (size -^ !soff)
in
       let buf = NBD.Buffer.alloc (Int64.to_int bs) in
       ignore (NBD.aio_pread_callback src buf !soff
-                (read_completed buf !soff));
+                ~completion:(read_completed buf !soff));
       soff := !soff +^ bs
     );
 
@@ -82,7 +82,8 @@ let asynch_copy src dst      List.iter (
       fun (buf, offset) ->
         (* Note the size of the write is implicitly stored in buf. *)
-        ignore (NBD.aio_pwrite_callback dst buf offset (write_completed buf))
+        ignore (NBD.aio_pwrite_callback dst buf offset
+                  ~completion:(write_completed buf))
     ) !writes;
     writes := [];
 
-- 
2.22.0

Commit 0904dd113dfa36485623b3c1756dc1aeab3dddeb removed the
theoretical possibility of clearing the debug callback from the handle
by setting it to NULL (although that was dubious from an API point of
view).

This commit adds an explicit way to do this.  Another advantage of
this is we can internally call this API from other places when we want
to clear/free the debug callback.
---
 generator/generator | 11 +++++++++++
 lib/debug.c         | 17 ++++++++++++++---
 lib/handle.c        |  4 +---
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/generator/generator b/generator/generator
index 6ccfc5f..d76eeea 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1013,6 +1013,17 @@ a handle then messages are printed on C<stderr>.
 
 The callback should not call C<nbd_*> APIs on the same handle since it
can
 be called while holding the handle lock and will cause a deadlock.";
+};
+
+  "clear_debug_callback", {
+    default_call with
+    args = [];
+    ret = RErr;
+    shortdesc = "clear the debug callback";
+    longdesc = "\
+Remove the debug callback if one was previously associated
+with the handle (with C<nbd_set_debug_callback>).  If not
+callback was associated this does nothing.";
 };
 
   "set_handle_name", {
diff --git a/lib/debug.c b/lib/debug.c
index ad4d9cb..c1decb2 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -38,13 +38,24 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
   return h->debug;
 }
 
+int
+nbd_unlocked_clear_debug_callback (struct nbd_handle *h)
+{
+  if (h->debug_callback)
+    /* ignore return value */
+    h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
+  h->debug_callback = NULL;
+  h->debug_data = NULL;
+  return 0;
+}
+
 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);
+  /* This can't fail at the moment - see implementation above. */
+  nbd_unlocked_clear_debug_callback (h);
+
   h->debug_callback = debug_callback;
   h->debug_data = data;
   return 0;
diff --git a/lib/handle.c b/lib/handle.c
index 054c8a7..d599eef 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -113,9 +113,7 @@ 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);
-  h->debug_callback = NULL;
+  nbd_unlocked_clear_debug_callback (h);
 
   free (h->bs_entries);
   for (m = h->meta_contexts; m != NULL; m = m_next) {
-- 
2.22.0

On 8/13/19 10:12 AM, Richard W.M. Jones wrote:
> An optional Closure parameter, but otherwise works the same way as
> Closure.
> ---
>  generator/generator | 57 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> @@ -4394,6 +4399,16 @@ let print_python_binding name { args; optargs; ret;
may_set_error } >    ) args;
>    List.iter (
>      function
> +    | OClosure { cbname } ->
> +       pr "  if (%s_user_data != 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 "      PyErr_SetString (PyExc_TypeError,\n";
> +       pr "                       \"callback parameter %s is not
callable\");\n" cbname;
> +       pr "      return NULL;\n";
Leaks %s_user_data, because we fail to do a matching Py_DECREF on
failure paths. It would be easier to defer the Py_INCREF to after we
verified it is Callable.  However, on looking further, I see this is
also a pre-existing bug affecting Closure.  For that matter, should we
be using 'py_ret = NULL; goto out;' similar to StringList, to avoid any
other leaks on any other failure paths?  (The generated python code
still probably has a number of poor handling of failures, which needs a
proper audit...)
> +       pr "    }\n";
> +       pr "  }\n"
>      | OFlags (n, _) -> pr "  %s_u32 = %s;\n" n n
>    ) optargs;
>  
> @@ -4423,6 +4438,9 @@ let print_python_binding name { args; optargs; ret;
may_set_error } >    ) args;
>    List.iter (
>      function
> +    | OClosure { cbname } ->
> +       pr ", %s_user_data ? %s_wrapper : NULL" cbname cbname;
Still looks wrong. %s_user_data is non-NULL; the check here should be
%s_user_data != Py_None.


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

On 8/13/19 10:12 AM, Richard W.M. Jones wrote:
> This doesn't change the C API, except that it is now permitted to pass
> NULL, NULL for the completion callback.  For the Python and OCaml
> APIs the parameter changes to be an optional parameter.
> ---
>  generator/generator                           | 36 +++++++++----------
>  ocaml/examples/asynch_copy.ml                 |  5 +--
>  .../test_505_aio_pread_structured_callback.ml | 12 ++++---
>  ocaml/tests/test_590_aio_copy.ml              |  5 +--
>  4 files changed, 30 insertions(+), 28 deletions(-)
> 
> diff --git a/generator/generator b/generator/generator
ACK

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

On 8/13/19 10:12 AM, Richard W.M. Jones wrote:
> Commit 0904dd113dfa36485623b3c1756dc1aeab3dddeb removed the
> theoretical possibility of clearing the debug callback from the handle
> by setting it to NULL (although that was dubious from an API point of
> view).
> 
> This commit adds an explicit way to do this.  Another advantage of
> this is we can internally call this API from other places when we want
> to clear/free the debug callback.
> ---
>  generator/generator | 11 +++++++++++
>  lib/debug.c         | 17 ++++++++++++++---
>  lib/handle.c        |  4 +---
>  3 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/generator/generator b/generator/generator
> index 6ccfc5f..d76eeea 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -1013,6 +1013,17 @@ a handle then messages are printed on
C<stderr>.
>  
>  The callback should not call C<nbd_*> APIs on the same handle since
it can
>  be called while holding the handle lock and will cause a deadlock.";
> +};
> +
> +  "clear_debug_callback", {
> +    default_call with
> +    args = [];
> +    ret = RErr;
Is RErr the right type, or can we make this a 'never fails' function?
> +    shortdesc = "clear the debug callback";
> +    longdesc = "\
> +Remove the debug callback if one was previously associated
> +with the handle (with C<nbd_set_debug_callback>).  If not
s/not/no/
> +callback was associated this does nothing.";
>  };
>  
>    "set_handle_name", {
> diff --git a/lib/debug.c b/lib/debug.c
> index ad4d9cb..c1decb2 100644
> --- a/lib/debug.c
> +++ b/lib/debug.c
> @@ -38,13 +38,24 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
>    return h->debug;
>  }
>  
> +int
> +nbd_unlocked_clear_debug_callback (struct nbd_handle *h)
> +{
> +  if (h->debug_callback)
> +    /* ignore return value */
> +    h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL,
NULL);
> +  h->debug_callback = NULL;
> +  h->debug_data = NULL;
> +  return 0;
> +}
Is it worth returning -1 if no callback was associated, and/or
forwarding the return value of callback(FREE, ptr) as the return value
of this function?  (That's more complicated; I'm also fine with always
returning 0 and ignoring the callback(FREE) return).

ACK with the typo fix.

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

On 8/13/19 10:12 AM, Richard W.M. Jones wrote:
> An optional Closure parameter, but otherwise works the same way as
> Closure.
> ---
>  generator/generator | 57 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 46 insertions(+), 11 deletions(-)
Do we need to edit the generated documentation strings to make it
obvious that passing NULL for an OClosure function is supported (and/or
the generated string for Closure that the function must not be NULL)?

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