thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH libnbd v2] generator: Define new Closure type instead of callbacks. [Jul 2019]

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2019-Jul-16 11:04 UTC

[Libguestfs] [PATCH libnbd v2] generator: Define new Closure type

As before, but this one has working Python bindings.  OCaml still TBD.

Rich.

Richard W.M. Jones

2019-Jul-16 11:04 UTC

head link

[Libguestfs] [PATCH libnbd v2] generator: Define new Closure type instead of callbacks.

A Closure is a list of (usually one, but can be more) closures.  In C
there is also a singe ‘void *user_data’ parameter which is passed by
the caller into the function and through as the first parameter of
each callback invocation.

By grouping the previously separate Opaque and Callback* parameters
together we can avoid the awkward situation where we have to scan
through the argument list to try to match them up.

Because this is a closure, in non-C languages we can simply map these
to a closure and drop the opaque parameter entirely.  It is not needed
since languages with proper closures can capture local state in the
closure.

Unlike the previous code it is no longer possible to mix persistent
and non-persistent callbacks in the same API call.  This was not used
before and is unlikely to be useful.

For the C API there is no API or ABI change (the only change is the
naming of the opaque pointer which is not part of the API).  For the
non-C languages the opaque parameter is no longer required as
discussed above.

Partly based on Eric Blake's earlier work here:
https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00160
---
 Makefile.am                         |   6 +-
 generator/generator                 | 619 +++++++++++++++-------------
 generator/states-reply-simple.c     |   2 +-
 generator/states-reply-structured.c |  12 +-
 generator/states-reply.c            |   2 +-
 generator/states.c                  |   3 +-
 lib/handle.c                        |   7 +-
 lib/internal.h                      |  11 +-
 lib/rw.c                            |  56 +--
 python/t/405-pread-structured.py    |  12 +-
 python/t/460-block-status.py        |  11 +-
 python/t/600-debug-callback.py      |   2 +-
 12 files changed, 391 insertions(+), 352 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 98a0284..d815a62 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -36,11 +36,13 @@ SUBDIRS = \
 	tests \
 	python \
 	sh \
-	ocaml \
-	ocaml/examples \
 	interop \
 	$(NULL)
 
+#	ocaml \
+#	ocaml/examples \
+#
+
 noinst_SCRIPTS = run
 
 # Check no files are missing from EXTRA_DIST rules, and that all
diff --git a/generator/generator b/generator/generator
index caa6353..d166c11 100755
--- a/generator/generator
+++ b/generator/generator
@@ -849,13 +849,14 @@ and arg                                written by the
function *)
 | BytesPersistIn of string * string (* same as above, but buffer persists *)
 | BytesPersistOut of string * string
-| Callback of string * arg list (* callback function returning int *)
-| CallbackPersist of string * arg list (* as above, but callback persists *)
+| Closure of bool * closure list (* void *opaque + one or more closures
+                              flag if true means callbacks persist
+                              in the handle, false means they only
+                              exist during the function call *)
 | Flags of string          (* NBD_CMD_FLAG_* flags *)
 | Int of string            (* small int *)
 | Int64 of string          (* 64 bit signed int *)
 | Mutable of arg           (* mutable argument, eg. int* *)
-| Opaque of string         (* opaque object, void* in C *)
 | Path of string           (* filename or path *)
 | SockAddrAndLen of string * string (* struct sockaddr * + socklen_t *)
 | String of string         (* string *)
@@ -863,6 +864,10 @@ and arg  | UInt of string           (* small unsigned int
*)
 | UInt32 of string         (* 32 bit unsigned int *)
 | UInt64 of string         (* 64 bit unsigned int *)
+and closure = {
+  cbname : string;         (* name of callback function *)
+  cbargs : arg list;       (* all closures return int for now *)
+}
 and ret  | RBool                    (* return a boolean, or error *)
 | RConstString             (* return a const string, NULL for error *)
@@ -915,9 +920,9 @@ Return the state of the debug flag on this handle.";
 
   "set_debug_callback", {
     default_call with
-    args = [ Opaque "data";
-             CallbackPersist ("debug_fn", [Opaque "data";
-                                           String "context"; String
"msg"]) ];
+    args = [ Closure (true,
+                      [{ cbname="debug_fn";
+                         cbargs=[String "context"; String
"msg"] }]) ];
     ret = RErr;
     shortdesc = "set the debug callback";
     longdesc = "\
@@ -1345,10 +1350,11 @@ protocol extensions).";
   "pread_structured", {
     default_call with
     args = [ BytesOut ("buf", "count"); UInt64
"offset";
-             Opaque "data";
-             Callback ("chunk", [ Opaque "data"; BytesIn
("subbuf", "count");
+             Closure (false,
+                      [{ cbname="chunk";
+                         cbargs=[ BytesIn ("subbuf",
"count");
                                   UInt64 "offset"; Int
"status";
-                                  Mutable (Int "error"); ]);
+                                  Mutable (Int "error")] }]);
              Flags "flags" ];
     ret = RErr;
     permitted_states = [ Connected ];
@@ -1534,12 +1540,13 @@ punching a hole.";
   "block_status", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Opaque "data";
-             Callback ("extent", [Opaque "data"; String
"metacontext";
-                                  UInt64 "offset";
-                                  ArrayAndLen (UInt32 "entries",
-                                               "nr_entries");
-                                  Mutable (Int "error")]);
+             Closure (false,
+                      [ {cbname="extent";
+                         cbargs=[String "metacontext";
+                                 UInt64 "offset";
+                                 ArrayAndLen (UInt32 "entries",
+                                              "nr_entries");
+                                 Mutable (Int "error")]} ]);
              Flags "flags" ];
     ret = RErr;
     permitted_states = [ Connected ];
@@ -1717,9 +1724,9 @@ C<nbd_pread>.";
   "aio_pread_callback", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Opaque "data";
-             CallbackPersist ("callback", [ Opaque "data";
Int64 "cookie";
-                                            Mutable (Int "error") ]);
+             Closure (true,
+                      [ {cbname="callback";
+                         cbargs=[Int64 "cookie"; Mutable (Int
"error")] } ]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1747,12 +1754,12 @@ cause a deadlock.";
   "aio_pread_structured", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Opaque "data";
-             CallbackPersist ("chunk", [ Opaque "data";
-                                         BytesIn ("subbuf",
"count");
-                                         UInt64 "offset";
-                                         Int "status";
-                                         Mutable (Int "error"); ]);
+             Closure (true,
+                      [ {cbname="chunk";
+                         cbargs=[BytesIn ("subbuf",
"count");
+                                 UInt64 "offset";
+                                 Int "status";
+                                 Mutable (Int "error");]} ]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1769,14 +1776,15 @@ documented in C<nbd_pread_structured>.";
   "aio_pread_structured_callback", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Opaque "data";
-             CallbackPersist ("chunk", [ Opaque "data";
-                                         BytesIn ("subbuf",
"count");
-                                         UInt64 "offset";
-                                         Int "status";
-                                         Mutable (Int "error"); ]);
-             CallbackPersist ("callback", [ Opaque "data";
Int64 "cookie";
-                                            Mutable (Int "error") ]);
+             Closure (true,
+                      [ {cbname="chunk";
+                         cbargs=[BytesIn ("subbuf",
"count");
+                                 UInt64 "offset";
+                                 Int "status";
+                                 Mutable (Int "error"); ]};
+                        {cbname="callback";
+                         cbargs=[Int64 "cookie";
+                                 Mutable (Int "error"); ]} ]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1819,9 +1827,9 @@ C<nbd_pwrite>.";
   "aio_pwrite_callback", {
     default_call with
     args = [ BytesPersistIn ("buf", "count"); UInt64
"offset";
-             Opaque "data";
-             CallbackPersist ("callback", [ Opaque "data";
Int64 "cookie";
-                                            Mutable (Int "error") ]);
+             Closure (true,
+                      [ {cbname="callback";
+                         cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1884,9 +1892,9 @@ Parameters behave as documented in
C<nbd_flush>.";
 
   "aio_flush_callback", {
     default_call with
-    args = [ Opaque "data";
-             CallbackPersist ("callback", [ Opaque "data";
Int64 "cookie";
-                                            Mutable (Int "error") ]);
+    args = [ Closure (true,
+                      [ {cbname="callback";
+                         cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1927,9 +1935,9 @@ Parameters behave as documented in
C<nbd_trim>.";
   "aio_trim_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Opaque "data";
-             CallbackPersist ("callback", [ Opaque "data";
Int64 "cookie";
-                                          Mutable (Int "error") ]);
+             Closure (true,
+                      [ {cbname="callback";
+                         cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1970,9 +1978,9 @@ Parameters behave as documented in
C<nbd_cache>.";
   "aio_cache_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Opaque "data";
-             CallbackPersist ("callback", [ Opaque "data";
Int64 "cookie";
-                                            Mutable (Int "error") ]);
+             Closure (true,
+                      [ {cbname="callback";
+                         cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2013,9 +2021,9 @@ Parameters behave as documented in
C<nbd_zero>.";
   "aio_zero_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Opaque "data";
-             CallbackPersist ("callback", [ Opaque "data";
Int64 "cookie";
-                                            Mutable (Int "error") ]);
+             Closure (true,
+                      [ {cbname="callback";
+                         cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2042,12 +2050,12 @@ cause a deadlock.";
   "aio_block_status", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Opaque "data";
-             CallbackPersist ("extent", [Opaque "data";
String "metacontext";
-                                         UInt64 "offset";
-                                         ArrayAndLen (UInt32
"entries",
-                                                      "nr_entries");
-                                         Mutable (Int "error")]);
+             Closure (true,
+                      [ {cbname="extent";
+                         cbargs=[String "metacontext"; UInt64
"offset";
+                                 ArrayAndLen (UInt32 "entries",
+                                              "nr_entries");
+                                 Mutable (Int "error")] } ]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2063,14 +2071,14 @@ Parameters behave as documented in
C<nbd_block_status>.";
   "aio_block_status_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Opaque "data";
-             CallbackPersist ("extent", [Opaque "data";
String "metacontext";
-                                         UInt64 "offset";
-                                         ArrayAndLen (UInt32
"entries",
-                                                      "nr_entries");
-                                         Mutable (Int "error") ]);
-             CallbackPersist ("callback", [ Opaque "data";
Int64 "cookie";
-                                            Mutable (Int "error") ]);
+             Closure (true,
+                      [ {cbname="extent";
+                         cbargs=[String "metacontext"; UInt64
"offset";
+                                 ArrayAndLen (UInt32 "entries",
+                                              "nr_entries");
+                                 Mutable (Int "error")]};
+                        {cbname="callback";
+                         cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -3143,19 +3151,19 @@ let generate_lib_libnbd_syms ()    pr "  local:
*;\n";
   pr "};\n"
 
-let rec name_of_arg = function
-| ArrayAndLen (arg, n) -> name_of_arg arg @ [n]
+let rec c_name_of_arg = function
+| ArrayAndLen (arg, n) -> c_name_of_arg arg @ [n]
 | Bool n -> [n]
 | BytesIn (n, len) -> [n; len]
 | BytesOut (n, len) -> [n; len]
 | BytesPersistIn (n, len) -> [n; len]
 | BytesPersistOut (n, len) -> [n; len]
-| Callback (n, _) | CallbackPersist (n, _) -> [n]
+| Closure (_, closures) ->
+   "user_data" :: List.map (fun { cbname } -> cbname) closures
 | Flags n -> [n]
 | Int n -> [n]
 | Int64 n -> [n]
-| Opaque n -> [n]
-| Mutable arg -> name_of_arg arg
+| Mutable arg -> c_name_of_arg arg
 | Path n -> [n]
 | SockAddrAndLen (n, len) -> [n; len]
 | String n -> [n]
@@ -3164,13 +3172,18 @@ let rec name_of_arg = function
 | UInt32 n -> [n]
 | UInt64 n -> [n]
 
-let rec print_c_arg_list ?(handle = false) args +let rec print_c_arg_list
?(handle = false) ?(user_data = false) args    pr "(";
   let comma = ref false in
   if handle then (
     comma := true;
     pr "struct nbd_handle *h";
   );
+  if user_data then (
+    if !comma then pr ", ";
+    comma := true;
+    pr "void *user_data";
+  );
   List.iter (
     fun arg ->
       if !comma then pr ", ";
@@ -3183,15 +3196,18 @@ let rec print_c_arg_list ?(handle = false) args        |
BytesPersistIn (n, len) -> pr "const void *%s, size_t %s" n len
       | BytesOut (n, len)
       | BytesPersistOut (n, len) -> pr "void *%s, size_t %s" n len
-      | Callback (n, args)
-      | CallbackPersist (n, args) ->
-         pr "int (*%s) " n; print_c_arg_list args
+      | Closure (_, cls) ->
+         pr "void *user_data";
+         List.iter (
+           fun { cbname; cbargs } ->
+             pr ", int (*%s) " cbname;
+             print_c_arg_list ~user_data:true cbargs
+         ) cls
       | Flags n -> pr "uint32_t %s" n
       | Int n -> pr "int %s" n
       | Int64 n -> pr "int64_t %s" n
       | Mutable (Int n) -> pr "int *%s" n
       | Mutable arg -> assert false
-      | Opaque n -> pr "void *%s" n
       | Path n
       | String n -> pr "const char *%s" n
       | StringList n -> pr "char **%s" n
@@ -3258,7 +3274,7 @@ let generate_include_libnbd_h ()    pr "\n";
   pr "struct nbd_handle;\n";
   pr "\n";
-  pr "typedef void (*nbd_close_callback) (void *data);\n";
+  pr "typedef void (*nbd_close_callback) (void *user_data);\n";
   pr "\n";
   List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i)
constants;
   pr "\n";
@@ -3275,7 +3291,8 @@ let generate_include_libnbd_h ()    pr "#define
LIBNBD_HAVE_NBD_GET_ERRNO 1\n";
   pr "\n";
   pr "extern int nbd_add_close_callback (struct nbd_handle *h,\n";
-  pr "                                   nbd_close_callback cb, void
*data);\n";
+  pr "                                   nbd_close_callback cb,\n";
+  pr "                                   void *user_data);\n";
   pr "#define LIBNBD_HAVE_NBD_ADD_CLOSE_CALLBACK 1\n";
   pr "\n";
   List.iter (
@@ -3386,7 +3403,7 @@ let generate_lib_api_c ()        pr "  }\n"
     );
     pr "  ret = nbd_unlocked_%s (h" name;
-    let argnames = List.flatten (List.map name_of_arg args) in
+    let argnames = List.flatten (List.map c_name_of_arg args) in
     List.iter (pr ", %s") argnames;
     pr ");\n";
     if permitted_states <> [] then
@@ -3560,9 +3577,9 @@ are called is not defined.  This API is only available
 from C and is designed to help when writing bindings to libnbd
 from other programming languages.
 
- typedef void (*nbd_close_callback) (void *data);
+ typedef void (*nbd_close_callback) (void *user_data);
  int nbd_add_close_callback (struct nbd_handle *nbd,
-                             nbd_close_callback cb, void *data);
+                             nbd_close_callback cb, void *user_data);
 
 ";
 
@@ -3709,159 +3726,160 @@ PyInit_libnbdmod (void)
 "
 
 let print_python_binding name { args; ret } -  (* Functions with a callback
parameter are special because we
-   * have to generate a wrapper function which translates the
-   * callback parameters back to Python.
+  (* Functions with a Closure parameter are special because we
+   * have to generate wrapper functions which translate the
+   * callbacks back to Python.
    *)
-  let find_callback opaque_id -    let cb -      try
-        List.find (
-          function
-          | Callback (_, args) | CallbackPersist (_, args) ->
-             List.mem (Opaque opaque_id) args
-          | _ -> false
-        ) args
-      with
-        Not_found ->
-        failwithf "%s: couldn't find callback associated with Opaque
%s"
-                  name opaque_id in
-    match cb with
-    | Callback (name, _) | CallbackPersist (name, _) -> name
-    | _ -> assert false
-  in
-
   List.iter (
     function
-    | Callback (cb_name, args) | CallbackPersist (cb_name, args) ->
-       pr "struct %s_%s_data {\n" name cb_name;
-       pr "  PyObject *fn;\n";
-       pr "  PyObject *data;\n";
+    | Closure (persistent, cls) ->
+       pr "struct %s_user_data {\n" name;
+       List.iter (
+         fun { cbname } ->
+           pr "  PyObject *%s;\n" cbname
+       ) cls;
        pr "};\n";
        pr "\n";
-       pr "static int\n";
-       pr "%s_%s_wrapper " name cb_name;
-       print_c_arg_list args;
-       pr "\n";
-       pr "{\n";
-       pr "  int ret;\n";
-       pr "  PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
-       pr "  PyObject *py_args, *py_ret;\n";
-       List.iter (
-         function
-         | ArrayAndLen (UInt32 n, len) ->
-            pr "  PyObject *py_%s = PyList_New (%s);\n" n len;
-            pr "  for (size_t i = 0; i < %s; ++i)\n" len;
-            pr "    PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong
(%s[i]));\n" n n
-         | BytesIn _
-         | Int _
-         | Int64 _ -> ()
-         | Mutable (Int n) ->
-            pr "  PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
-            pr "  if (!py_%s_modname) { PyErr_PrintEx (0); return -1;
}\n" n;
-            pr "  PyObject *py_%s_mod = PyImport_Import
(py_%s_modname);\n" n n;
-            pr "  Py_DECREF (py_%s_modname);\n" n;
-            pr "  if (!py_%s_mod) { PyErr_PrintEx (0); return -1;
}\n" n;
-            pr "  PyObject *py_%s = PyObject_CallMethod (py_%s_mod,
\"c_int\", \"i\", *%s);\n" n n n;
-            pr "  if (!py_%s) { PyErr_PrintEx (0); return -1; }\n" n;
-         | Opaque n ->
-            pr "  struct %s_%s_data *_data = %s;\n" name cb_name n
-         | String n
-         | UInt64 n -> ()
-         (* The following not yet implemented for callbacks XXX *)
-         | ArrayAndLen _ | Bool _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _
-         | Callback _ | CallbackPersist _
-         | Flags _ | Mutable _
-         | Path _ | SockAddrAndLen _ | StringList _
-         | UInt _ | UInt32 _ -> assert false
-       ) args;
-       pr "\n";
 
-       pr "  py_args = Py_BuildValue (\"(\"";
-       List.iter (
-         function
-         | ArrayAndLen (UInt32 n, len) -> pr " \"O\""
-         | BytesIn (n, len) -> pr " \"y#\""
-         | Int n -> pr " \"i\""
-         | Int64 n -> pr " \"L\""
-         | Mutable (Int n) -> pr " \"O\""
-         | Opaque n -> pr " \"O\""
-         | String n -> pr " \"s\""
-         | UInt64 n -> pr " \"K\""
-         (* The following not yet implemented for callbacks XXX *)
-         | ArrayAndLen _ | Bool _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _
-         | Callback _ | CallbackPersist _
-         | Flags _ | Mutable _
-         | Path _ | SockAddrAndLen _ | StringList _
-         | UInt _ | UInt32 _ -> assert false
-       ) args;
-       pr " \")\"";
-       List.iter (
-         function
-         | ArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
-         | BytesIn (n, len) -> pr ", %s, (int) %s" n len
-         | Mutable (Int n) -> pr ", py_%s" n
-         | Opaque _ -> pr ", _data->data"
-         | Int n | Int64 n
-         | String n
-         | UInt64 n -> pr ", %s" n
-         (* The following not yet implemented for callbacks XXX *)
-         | ArrayAndLen _ | Bool _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _
-         | Callback _ | CallbackPersist _
-         | Flags _ | Mutable _
-         | Path _ | SockAddrAndLen _ | StringList _
-         | UInt _ | UInt32 _ -> assert false
-       ) args;
-       pr ");\n";
-       pr "  Py_INCREF (py_args);\n";
-       pr "\n";
-       pr "  if (PyEval_ThreadsInitialized ())\n";
-       pr "    py_save = PyGILState_Ensure ();\n";
-       pr "\n";
-       pr "  py_ret = PyObject_CallObject (_data->fn,
py_args);\n";
-       pr "\n";
-       pr "  if (PyEval_ThreadsInitialized ())\n";
-       pr "    PyGILState_Release (py_save);\n";
-       pr "\n";
-       pr "  Py_DECREF (py_args);\n";
-       pr "\n";
-       pr "  if (py_ret != NULL) {\n";
-       pr "    ret = 0;\n";
-       pr "    Py_DECREF (py_ret); /* return value is discarded
*/\n";
-       pr "  }\n";
-       pr "  else {\n";
-       pr "    ret = -1;\n";
-       pr "    PyErr_PrintEx (0); /* print exception */\n";
-       pr "  };\n";
-       pr "\n";
+       (* Persistent closures need an explicit function to decrement
+        * the closure refcounts and free the user_data struct.
+        *)
+       if persistent then (
+         pr "static void\n";
+         pr "free_%s_user_data (void *vp)\n" name;
+         pr "{\n";
+         pr "  struct %s_user_data *user_data = vp;\n" name;
+         pr "\n";
+         List.iter (
+           fun { cbname } ->
+             pr "  Py_DECREF (user_data->%s);\n" cbname
+         ) cls;
+         pr "  free (user_data);\n";
+         pr "}\n";
+         pr "\n";
+       );
+
        List.iter (
-         function
-         | ArrayAndLen (UInt32 n, _) ->
-            pr "  Py_DECREF (py_%s);\n" n
-         | Mutable (Int n) ->
-            pr "  PyObject *py_%s_ret = PyObject_GetAttrString (py_%s,
\"value\");\n" n n;
-            pr "  *%s = PyLong_AsLong (py_%s_ret);\n" n n;
-            pr "  Py_DECREF (py_%s_ret);\n" n;
-            pr "  Py_DECREF (py_%s);\n" n
-         | BytesIn _
-         | Int _ | Int64 _
-         | Opaque _
-         | String _
-         | UInt64 _ -> ()
-         (* The following not yet implemented for callbacks XXX *)
-         | ArrayAndLen _ | Bool _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _
-         | Callback _ | CallbackPersist _
-         | Flags _ | Mutable _
-         | Path _ | SockAddrAndLen _ | StringList _
-         | UInt _ | UInt32 _ -> assert false
-       ) args;
-       pr "  return ret;\n";
-       pr "}\n";
-       pr "\n"
+         fun { cbname; cbargs } ->
+           pr "static int\n";
+           pr "%s_%s_wrapper " name cbname;
+           print_c_arg_list ~user_data:true cbargs;
+           pr "\n";
+           pr "{\n";
+           pr "  int ret;\n";
+           pr "  PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
+           pr "  PyObject *py_args, *py_ret;\n";
+           List.iter (
+             function
+             | ArrayAndLen (UInt32 n, len) ->
+                pr "  PyObject *py_%s = PyList_New (%s);\n" n len;
+                pr "  for (size_t i = 0; i < %s; ++i)\n" len;
+                pr "    PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong
(%s[i]));\n" n n
+             | BytesIn _
+             | Int _
+             | Int64 _ -> ()
+             | Mutable (Int n) ->
+                pr "  PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
+                pr "  if (!py_%s_modname) { PyErr_PrintEx (0); return -1;
}\n" n;
+                pr "  PyObject *py_%s_mod = PyImport_Import
(py_%s_modname);\n" n n;
+                pr "  Py_DECREF (py_%s_modname);\n" n;
+                pr "  if (!py_%s_mod) { PyErr_PrintEx (0); return -1;
}\n" n;
+                pr "  PyObject *py_%s = PyObject_CallMethod (py_%s_mod,
\"c_int\", \"i\", *%s);\n" n n n;
+                pr "  if (!py_%s) { PyErr_PrintEx (0); return -1;
}\n" n;
+             | String n
+             | UInt64 n -> ()
+             (* The following not yet implemented for callbacks XXX *)
+             | ArrayAndLen _ | Bool _ | BytesOut _
+             | BytesPersistIn _ | BytesPersistOut _
+             | Closure _
+             | Flags _ | Mutable _
+             | Path _ | SockAddrAndLen _ | StringList _
+             | UInt _ | UInt32 _ -> assert false
+           ) cbargs;
+           pr "\n";
+
+           pr "  py_args = Py_BuildValue (\"(\"";
+           List.iter (
+             function
+             | ArrayAndLen (UInt32 n, len) -> pr "
\"O\""
+             | BytesIn (n, len) -> pr " \"y#\""
+             | Int n -> pr " \"i\""
+             | Int64 n -> pr " \"L\""
+             | Mutable (Int n) -> pr " \"O\""
+             | String n -> pr " \"s\""
+             | UInt64 n -> pr " \"K\""
+             (* The following not yet implemented for callbacks XXX *)
+             | ArrayAndLen _ | Bool _ | BytesOut _
+             | BytesPersistIn _ | BytesPersistOut _
+             | Closure _
+             | Flags _ | Mutable _
+             | Path _ | SockAddrAndLen _ | StringList _
+             | UInt _ | UInt32 _ -> assert false
+           ) cbargs;
+           pr " \")\"";
+           List.iter (
+             function
+             | ArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
+             | BytesIn (n, len) -> pr ", %s, (int) %s" n len
+             | Mutable (Int n) -> pr ", py_%s" n
+             | Int n | Int64 n
+             | String n
+             | UInt64 n -> pr ", %s" n
+             (* The following not yet implemented for callbacks XXX *)
+             | ArrayAndLen _ | Bool _ | BytesOut _
+             | BytesPersistIn _ | BytesPersistOut _
+             | Closure _
+             | Flags _ | Mutable _
+             | Path _ | SockAddrAndLen _ | StringList _
+             | UInt _ | UInt32 _ -> assert false
+           ) cbargs;
+           pr ");\n";
+           pr "  Py_INCREF (py_args);\n";
+           pr "\n";
+           pr "  if (PyEval_ThreadsInitialized ())\n";
+           pr "    py_save = PyGILState_Ensure ();\n";
+           pr "\n";
+           pr "  py_ret = PyObject_CallObject (((struct %s_user_data
*)user_data)->%s, py_args);\n" name cbname;
+           pr "\n";
+           pr "  if (PyEval_ThreadsInitialized ())\n";
+           pr "    PyGILState_Release (py_save);\n";
+           pr "\n";
+           pr "  Py_DECREF (py_args);\n";
+           pr "\n";
+           pr "  if (py_ret != NULL) {\n";
+           pr "    ret = 0;\n";
+           pr "    Py_DECREF (py_ret); /* return value is discarded
*/\n";
+           pr "  }\n";
+           pr "  else {\n";
+           pr "    ret = -1;\n";
+           pr "    PyErr_PrintEx (0); /* print exception */\n";
+           pr "  };\n";
+           pr "\n";
+           List.iter (
+             function
+             | ArrayAndLen (UInt32 n, _) ->
+                pr "  Py_DECREF (py_%s);\n" n
+             | Mutable (Int n) ->
+                pr "  PyObject *py_%s_ret = PyObject_GetAttrString (py_%s,
\"value\");\n" n n;
+                pr "  *%s = PyLong_AsLong (py_%s_ret);\n" n n;
+                pr "  Py_DECREF (py_%s_ret);\n" n;
+                pr "  Py_DECREF (py_%s);\n" n
+             | BytesIn _
+             | Int _ | Int64 _
+             | String _
+             | UInt64 _ -> ()
+             (* The following not yet implemented for callbacks XXX *)
+             | ArrayAndLen _ | Bool _ | BytesOut _
+             | BytesPersistIn _ | BytesPersistOut _
+             | Closure _
+             | Flags _ | Mutable _
+             | Path _ | SockAddrAndLen _ | StringList _
+             | UInt _ | UInt32 _ -> assert false
+           ) cbargs;
+           pr "  return ret;\n";
+           pr "}\n";
+           pr "\n"
+       ) cls
     | _ -> ()
   ) args;
 
@@ -3899,10 +3917,11 @@ let print_python_binding name { args; ret }         pr
"  PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n"
           n;
        pr "  struct py_aio_buffer *%s_buf;\n" n
-    | Callback (n, _) ->
-       pr "  struct %s_%s_data _%s_data, *%s_data = &_%s_data;\n"
name n n n n
-    | CallbackPersist (n, _) ->
-       pr "  struct %s_%s_data *%s_data;\n" name n n
+    | Closure (false, cls) ->
+       pr "  struct %s_user_data _user_data;\n" name;
+       pr "  struct %s_user_data *user_data = &_user_data;\n"
name
+    | Closure (true, cls) ->
+       pr "  struct %s_user_data *user_data;\n" name
     | Flags n ->
        pr "  uint32_t %s_u32;\n" n;
        pr "  unsigned int %s; /* really uint32_t */\n" n
@@ -3911,8 +3930,7 @@ let print_python_binding name { args; ret }         pr
"  int64_t %s_i64;\n" n;
        pr "  long long %s; /* really int64_t */\n" n
     | Mutable arg ->
-       pr "  PyObject *%s;\n" (List.hd (name_of_arg arg))
-    | Opaque _ -> ()
+       pr "  PyObject *%s;\n" (List.hd (c_name_of_arg arg))
     | Path n ->
        pr "  PyObject *py_%s = NULL;\n" n;
        pr "  char *%s = NULL;\n" n
@@ -3935,34 +3953,17 @@ let print_python_binding name { args; ret }    ) args;
   pr "\n";
 
-  (* Allocate the parameter. *)
+  (* Allocate the persistent closure user_data. *)
   List.iter (
     function
-    | ArrayAndLen _
-    | Bool _
-    | BytesIn _
-    | BytesPersistIn _
-    | BytesOut _
-    | BytesPersistOut _
-    | Callback _ -> ()
-    | CallbackPersist (n, _) ->
-       pr "  %s_data = malloc (sizeof *%s_data);\n" n n;
-       pr "  if (%s_data == NULL) {\n" n;
+    | Closure (false, _) -> ()
+    | Closure (true, cls) ->
+       pr "  user_data = malloc (sizeof *user_data);\n";
+       pr "  if (user_data == NULL) {\n";
        pr "    PyErr_NoMemory ();\n";
        pr "    return NULL;\n";
        pr "  }\n"
-    | Flags _
-    | Int _
-    | Int64 _
-    | Opaque _
-    | Mutable _
-    | Path _
-    | SockAddrAndLen _
-    | String _
-    | StringList _
-    | UInt _
-    | UInt32 _
-    | UInt64 _ -> ()
+    | _ -> ()
   ) args;
 
   (* Parse the Python parameters. *)
@@ -3975,12 +3976,11 @@ let print_python_binding name { args; ret }      |
BytesPersistIn (n, _) -> pr " \"O\""
     | BytesOut (_, count) -> pr " \"n\""
     | BytesPersistOut (_, count) -> pr " \"O\""
-    | Callback (n, _) | CallbackPersist (n, _) -> pr "
\"O\""
+    | Closure (_, cls) -> List.iter (fun _ -> pr "
\"O\"") cls
     | Flags n -> pr " \"I\""
     | Int n -> pr " \"i\""
     | Int64 n -> pr " \"L\""
     | Mutable _ -> pr " \"O\""
-    | Opaque _ -> pr " \"O\""
     | Path n -> pr " \"O&\""
     | SockAddrAndLen (n, _) -> pr " \"O\""
     | String n -> pr " \"s\""
@@ -4000,12 +4000,12 @@ let print_python_binding name { args; ret }      |
BytesIn (n, _) | BytesPersistIn (n, _)
     | BytesPersistOut (n, _) -> pr ", &%s" n
     | BytesOut (_, count) -> pr ", &%s" count
-    | Callback (n, _) | CallbackPersist (n, _) -> pr ",
&%s_data->fn" n
+    | Closure (_, cls) ->
+       List.iter (fun { cbname } -> pr ", &user_data->%s"
cbname) cls
     | Flags n -> pr ", &%s" n
     | Int n -> pr ", &%s" n
     | Int64 n -> pr ", &%s" n
-    | Mutable arg -> pr ", &%s" (List.hd (name_of_arg arg))
-    | Opaque n -> pr ", &%s_data->data" (find_callback n)
+    | Mutable arg -> pr ", &%s" (List.hd (c_name_of_arg arg))
     | Path n -> pr ", PyUnicode_FSConverter, &py_%s" n
     | SockAddrAndLen (n, _) -> pr ", &%s" n
     | String n -> pr ", &%s" n
@@ -4017,6 +4017,20 @@ let print_python_binding name { args; ret }    pr
"))\n";
   pr "    return NULL;\n";
 
+  (* If the parameter has persistent closures then we need to
+   * make sure the ref count remains positive.
+   *)
+  List.iter (
+    function
+    | Closure (false, _) -> ()
+    | Closure (true, cls) ->
+       List.iter (
+         fun { cbname } ->
+           pr "  Py_INCREF (user_data->%s);\n" cbname
+       ) cls
+    | _ -> ()
+  ) args;
+
   pr "  h = get_handle (py_h);\n";
   List.iter (
     function
@@ -4044,19 +4058,20 @@ let print_python_binding name { args; ret }         pr
"  %s = malloc (%s);\n" n count
     | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
        pr "  %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n
-    | Callback (n, _) | CallbackPersist (n, _) ->
-       pr "  if (!PyCallable_Check (%s_data->fn)) {\n" n;
-       pr "    PyErr_SetString (PyExc_TypeError,\n";
-       pr "                     \"callback parameter %s is not
callable\");\n"
-          n;
-       pr "    return NULL;\n";
-       pr "  }\n"
+    | Closure (_, cls) ->
+       List.iter (
+         fun { cbname } ->
+           pr "  if (!PyCallable_Check (user_data->%s)) {\n"
cbname;
+           pr "    PyErr_SetString (PyExc_TypeError,\n";
+           pr "                     \"callback parameter %s is not
callable\");\n" cbname;
+           pr "    return NULL;\n";
+           pr "  }\n"
+       ) cls
     | Flags n -> pr "  %s_u32 = %s;\n" n n
     | Int _ -> ()
     | Int64 n -> pr "  %s_i64 = %s;\n" n n
     | Mutable _ ->
        pr "  abort (); /* Mutable for normal Python parameters not impl
*/\n"
-    | Opaque n -> ()
     | Path n ->
        pr "  %s = PyBytes_AS_STRING (py_%s);\n" n n;
        pr "  assert (%s != NULL);\n" n
@@ -4082,12 +4097,16 @@ let print_python_binding name { args; ret }      |
BytesOut (n, count) -> pr ", %s, %s" n count
     | BytesPersistIn (n, _)
     | BytesPersistOut (n, _) -> pr ", %s_buf->data,
%s_buf->len" n n
-    | Callback (n, _) | CallbackPersist (n, _) -> pr ",
%s_%s_wrapper" name n
+    | Closure (_, cls) ->
+       pr ", user_data";
+       List.iter (
+         fun { cbname } ->
+           pr ", %s_%s_wrapper" name cbname
+       ) cls
     | Flags n -> pr ", %s_u32" n
     | Int n -> pr ", %s" n
     | Int64 n -> pr ", %s_i64" n
-    | Mutable arg -> pr ", /* XXX */ (void *) %s" (List.hd
(name_of_arg arg))
-    | Opaque n -> pr ", %s_data" (find_callback n)
+    | Mutable arg -> pr ", /* XXX */ (void *) %s" (List.hd
(c_name_of_arg arg))
     | Path n -> pr ", %s" n
     | SockAddrAndLen (n, _) -> pr ", /* XXX */ (void *) %s, 0" n
     | String n -> pr ", %s" n
@@ -4117,12 +4136,11 @@ let print_python_binding name { args; ret }      | Bool
_
     | BytesIn _
     | BytesPersistIn _ | BytesPersistOut _
-    | Callback _ | CallbackPersist _
+    | Closure _
     | Flags _
     | Int _
     | Int64 _
     | Mutable _
-    | Opaque _
     | Path _
     | SockAddrAndLen _
     | String _
@@ -4160,15 +4178,14 @@ let print_python_binding name { args; ret }      | Bool
_ -> ()
     | BytesIn (n, _) -> pr "  PyBuffer_Release (&%s);\n" n
     | BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> ()
-    | Callback _ -> ()
-    | CallbackPersist (n, _) ->
+    | Closure (false, _) -> ()
+    | Closure (true, _) ->
        pr "  /* This ensures the callback data is freed eventually.
*/\n";
-       pr "  nbd_add_close_callback (h, free, %s_data);\n" n
+       pr "  nbd_add_close_callback (h, free_%s_user_data,
user_data);\n" name
     | Flags _ -> ()
     | Int _ -> ()
     | Int64 _ -> ()
     | Mutable _ -> ()
-    | Opaque _ -> ()
     | Path n ->
        pr "  Py_XDECREF (py_%s);\n" n
     | SockAddrAndLen _ -> ()
@@ -4302,28 +4319,30 @@ class NBD (object):
 
   List.iter (
     fun (name, { args; shortdesc; longdesc }) ->
-      let args = List.map (
-                     function
-                     | ArrayAndLen (UInt32 n, _) -> n, None
-                     | ArrayAndLen _ -> assert false
-                     | Bool n -> n, None
-                     | BytesIn (n, _) | BytesPersistIn (n, _) -> n, None
-                     | BytesPersistOut (n, _) -> n, None
-                     | BytesOut (_, count) -> count, None
-                     | Callback (n, _) | CallbackPersist (n, _) -> n, None
-                     | Flags n -> n, Some "0"
-                     | Int n -> n, None
-                     | Int64 n -> n, None
-                     | Mutable arg -> List.hd (name_of_arg arg), None
-                     | Opaque n -> n, None
-                     | Path n -> n, None
-                     | SockAddrAndLen (n, _) -> n, None
-                     | String n -> n, None
-                     | StringList n -> n, None
-                     | UInt n -> n, None
-                     | UInt32 n -> n, None
-                     | UInt64 n -> n, None
-                   ) args in
+      let args +        List.map (
+            function
+            | ArrayAndLen (UInt32 n, _) -> [n, None]
+            | ArrayAndLen _ -> assert false
+            | Bool n -> [n, None]
+            | BytesIn (n, _) | BytesPersistIn (n, _) -> [n, None]
+            | BytesPersistOut (n, _) -> [n, None]
+            | BytesOut (_, count) -> [count, None]
+            | Closure (_, cls) -> 
+               List.map (fun { cbname } -> cbname, None) cls
+            | Flags n -> [n, Some "0"]
+            | Int n -> [n, None]
+            | Int64 n -> [n, None]
+            | Mutable arg -> [List.hd (c_name_of_arg arg), None]
+            | Path n -> [n, None]
+            | SockAddrAndLen (n, _) -> [n, None]
+            | String n -> [n, None]
+            | StringList n -> [n, None]
+            | UInt n -> [n, None]
+            | UInt32 n -> [n, None]
+            | UInt64 n -> [n, None]
+        ) args in
+      let args = List.flatten args in
       let ()          let args = List.map (
           function
@@ -4357,6 +4376,7 @@ if __name__ == \"__main__\":
     nbdsh.shell()
 "
 
+(*
 (*----------------------------------------------------------------------*)
 
 (* OCaml bindings. *)
@@ -4427,7 +4447,7 @@ and ocaml_ret_to_string = function
 let name_of_ocaml_arg = function
   | OCamlHandle -> "h"
   | OCamlFlags n -> n
-  | OCamlArg a -> List.hd (name_of_arg a)
+  | OCamlArg a -> List.hd (c_name_of_arg a)
 
 let generate_ocaml_nbd_mli ()    generate_header OCamlStyle;
@@ -4686,7 +4706,7 @@ let print_ocaml_binding (name, { args; ret })         pr
"  int ret;\n";
        pr "\n";
        pr "  caml_leave_blocking_section ();\n";
-       let c_argnames = List.flatten (List.map name_of_arg args) in
+       let c_argnames = List.flatten (List.map c_name_of_arg args) in
        pr "  ret = %s_%s_wrapper_locked (%s);\n" name cb_name
           (String.concat ", " c_argnames);
        pr "  caml_enter_blocking_section ();\n";
@@ -4845,7 +4865,7 @@ let print_ocaml_binding (name, { args; ret })    pr "
r =  nbd_%s (h%s);\n" name
      (String.concat ""
                     (List.map ((^) ", ")
-                              (List.flatten (List.map name_of_arg args))));
+                              (List.flatten (List.map c_name_of_arg args))));
   pr "  caml_leave_blocking_section ();\n";
   pr "\n";
   pr "  if (r == %s)\n" errcode;
@@ -4938,6 +4958,7 @@ let generate_ocaml_nbd_c ()    pr "\n";
 
   List.iter print_ocaml_binding handle_calls
+*)
 
 (*----------------------------------------------------------------------*)
 
@@ -4954,6 +4975,8 @@ let ()    output_to "python/libnbdmod.c"
generate_python_libnbdmod_c;
   output_to "python/methods.c" generate_python_methods_c;
   output_to "python/nbd.py" generate_python_nbd_py;
+(*
   output_to "ocaml/NBD.mli" generate_ocaml_nbd_mli;
   output_to "ocaml/NBD.ml" generate_ocaml_nbd_ml;
   output_to "ocaml/nbd-c.c" generate_ocaml_nbd_c;
+ *)
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index ba26f8c..72a401b 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -64,7 +64,7 @@
       int error = 0;
 
       assert (cmd->error == 0);
-      if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data, cmd->count,
+      if (cmd->cb.fn.read (cmd->cb.user_data, cmd->data,
cmd->count,
                            cmd->offset, LIBNBD_READ_DATA, &error) == -1)
         cmd->error = error ? error : EPROTO;
     }
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 5c4d2f2..3168d1b 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -298,7 +298,8 @@
          * current error rather than any earlier one. If the callback fails
          * without setting errno, then use the server's error below.
          */
-        if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + (offset -
cmd->offset),
+        if (cmd->cb.fn.read (cmd->cb.user_data,
+                             cmd->data + (offset - cmd->offset),
                              0, offset, LIBNBD_READ_ERROR, &scratch) == -1)
           if (cmd->error == 0)
             cmd->error = scratch;
@@ -384,7 +385,8 @@
     if (cmd->cb.fn.read) {
       int error = cmd->error;
 
-      if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + (offset -
cmd->offset),
+      if (cmd->cb.fn.read (cmd->cb.user_data,
+                           cmd->data + (offset - cmd->offset),
                            length - sizeof offset, offset,
                            LIBNBD_READ_DATA, &error) == -1)
         if (cmd->error == 0)
@@ -445,7 +447,8 @@
     if (cmd->cb.fn.read) {
       int error = cmd->error;
 
-      if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + offset, length,
+      if (cmd->cb.fn.read (cmd->cb.user_data,
+                           cmd->data + offset, length,
                            cmd->offset + offset,
                            LIBNBD_READ_HOLE, &error) == -1)
         if (cmd->error == 0)
@@ -496,7 +499,8 @@
       /* Call the caller's extent function. */
       int error = cmd->error;
 
-      if (cmd->cb.fn.extent (cmd->cb.opaque, meta_context->name,
cmd->offset,
+      if (cmd->cb.fn.extent (cmd->cb.user_data,
+                             meta_context->name, cmd->offset,
                              &h->bs_entries[1], (length-4) / 4,
&error) == -1)
         if (cmd->error == 0)
           cmd->error = error ? error : EPROTO;
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 742fc1a..ebdbdc1 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -184,7 +184,7 @@ save_reply_state (struct nbd_handle *h)
   if (cmd->cb.callback) {
     int error = cmd->error;
 
-    if (cmd->cb.callback (cmd->cb.opaque, cookie, &error) == -1
&& error)
+    if (cmd->cb.callback (cmd->cb.user_data, cookie, &error) == -1
&& error)
       cmd->error = error;
   }
 
diff --git a/generator/states.c b/generator/states.c
index 992f833..93ad4e5 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -124,7 +124,8 @@ void abort_commands (struct nbd_handle *h,
       int error = cmd->error ? cmd->error : ENOTCONN;
 
       assert (cmd->type != NBD_CMD_DISC);
-      if (cmd->cb.callback (cmd->cb.opaque, cmd->cookie, &error)
== -1 && error)
+      if (cmd->cb.callback (cmd->cb.user_data, cmd->cookie,
+                            &error) == -1 && error)
         cmd->error = error;
     }
     if (cmd->error == 0)
diff --git a/lib/handle.c b/lib/handle.c
index cbe7e8a..5003227 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -98,7 +98,7 @@ nbd_close (struct nbd_handle *h)
 
   for (cc = h->close_callbacks; cc != NULL; cc = cc_next) {
     cc_next = cc->next;
-    cc->cb (cc->data);
+    cc->cb (cc->user_data);
     free (cc);
   }
 
@@ -202,7 +202,8 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const
char *name)
  * programming languages.
  */
 int
-nbd_add_close_callback (struct nbd_handle *h, nbd_close_callback cb, void
*data)
+nbd_add_close_callback (struct nbd_handle *h,
+                        nbd_close_callback cb, void *user_data)
 {
   int ret;
   struct close_callback *cc;
@@ -216,7 +217,7 @@ nbd_add_close_callback (struct nbd_handle *h,
nbd_close_callback cb, void *data)
   }
   cc->next = h->close_callbacks;
   cc->cb = cb;
-  cc->data = data;
+  cc->user_data = user_data;
   h->close_callbacks = cc;
 
   ret = 0;
diff --git a/lib/internal.h b/lib/internal.h
index 44272b9..347bf69 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -212,7 +212,7 @@ struct meta_context {
 struct close_callback {
   struct close_callback *next;  /* Linked list. */
   nbd_close_callback cb;        /* Function. */
-  void *data;                   /* Data. */
+  void *user_data;              /* Data. */
 };
 
 struct socket_ops {
@@ -241,14 +241,15 @@ struct socket {
   const struct socket_ops *ops;
 };
 
-typedef int (*extent_fn) (void *data, const char *metacontext, uint64_t offset,
+typedef int (*extent_fn) (void *user_data,
+                          const char *metacontext, uint64_t offset,
                           uint32_t *entries, size_t nr_entries, int *error);
-typedef int (*read_fn) (void *data, const void *buf, size_t count,
+typedef int (*read_fn) (void *user_data, const void *buf, size_t count,
                         uint64_t offset, int status, int *error);
-typedef int (*callback_fn) (void *data, int64_t cookie, int *error);
+typedef int (*callback_fn) (void *user_data, int64_t cookie, int *error);
 
 struct command_cb {
-  void *opaque;
+  void *user_data;
   union {
     extent_fn extent;
     read_fn read;
diff --git a/lib/rw.c b/lib/rw.c
index a878fea..f2fe4e0 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -60,12 +60,12 @@ 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,
-                               void *opaque, read_fn read, uint32_t flags)
+                               void *user_data, read_fn read, uint32_t flags)
 {
   int64_t ch;
 
   ch = nbd_unlocked_aio_pread_structured (h, buf, count, offset,
-                                          opaque, read, flags);
+                                          user_data, read, flags);
   if (ch == -1)
     return -1;
 
@@ -145,11 +145,12 @@ nbd_unlocked_zero (struct nbd_handle *h,
 int
 nbd_unlocked_block_status (struct nbd_handle *h,
                            uint64_t count, uint64_t offset,
-                           void *data, extent_fn extent, uint32_t flags)
+                           void *user_data, extent_fn extent, uint32_t flags)
 {
   int64_t ch;
 
-  ch = nbd_unlocked_aio_block_status (h, count, offset, data, extent, flags);
+  ch = nbd_unlocked_aio_block_status (h, count, offset, user_data, extent,
+                                      flags);
   if (ch == -1)
     return -1;
 
@@ -159,8 +160,8 @@ nbd_unlocked_block_status (struct nbd_handle *h,
 int64_t
 nbd_internal_command_common (struct nbd_handle *h,
                              uint16_t flags, uint16_t type,
-                             uint64_t offset, uint64_t count, void *data,
-                             struct command_cb *cb)
+                             uint64_t offset, uint64_t count,
+                             void *data, struct command_cb *cb)
 {
   struct command_in_flight *cmd, *prev_cmd;
 
@@ -256,10 +257,10 @@ 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,
-                                 void *opaque, callback_fn callback,
+                                 void *user_data, callback_fn callback,
                                  uint32_t flags)
 {
-  struct command_cb cb = { .opaque = opaque, .callback = callback, };
+  struct command_cb cb = { .user_data = user_data, .callback = callback, };
 
   /* We could silently accept flag DF, but it really only makes sense
    * with callbacks, because otherwise there is no observable change
@@ -277,20 +278,22 @@ 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,
-                                   void *opaque, read_fn read, uint32_t flags)
+                                   void *user_data, read_fn read,
+                                   uint32_t flags)
 {
   return nbd_unlocked_aio_pread_structured_callback (h, buf, count, offset,
-                                                     opaque, read, NULL,
flags);
+                                                     user_data, read, NULL,
+                                                     flags);
 }
 
 int64_t
 nbd_unlocked_aio_pread_structured_callback (struct nbd_handle *h, void *buf,
                                             size_t count, uint64_t offset,
-                                            void *opaque, read_fn read,
+                                            void *user_data, read_fn read,
                                             callback_fn callback,
                                             uint32_t flags)
 {
-  struct command_cb cb = { .opaque = opaque, .fn.read = read,
+  struct command_cb cb = { .user_data = user_data, .fn.read = read,
                            .callback = callback, };
 
   if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
@@ -320,10 +323,10 @@ 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,
-                                  void *opaque, callback_fn callback,
+                                  void *user_data, callback_fn callback,
                                   uint32_t flags)
 {
-  struct command_cb cb = { .opaque = opaque, .callback = callback, };
+  struct command_cb cb = { .user_data = user_data, .callback = callback, };
 
   if (nbd_unlocked_read_only (h) == 1) {
     set_error (EINVAL, "server does not support write operations");
@@ -352,10 +355,10 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, uint32_t
flags)
 }
 
 int64_t
-nbd_unlocked_aio_flush_callback (struct nbd_handle *h, void *opaque,
+nbd_unlocked_aio_flush_callback (struct nbd_handle *h, void *user_data,
                                  callback_fn callback, uint32_t flags)
 {
-  struct command_cb cb = { .opaque = opaque, .callback = callback, };
+  struct command_cb cb = { .user_data = user_data, .callback = callback, };
 
   if (nbd_unlocked_can_flush (h) != 1) {
     set_error (EINVAL, "server does not support flush operations");
@@ -382,10 +385,10 @@ 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,
-                                void *opaque, callback_fn callback,
+                                void *user_data, callback_fn callback,
                                 uint32_t flags)
 {
-  struct command_cb cb = { .opaque = opaque, .callback = callback, };
+  struct command_cb cb = { .user_data = user_data, .callback = callback, };
 
   if (nbd_unlocked_read_only (h) == 1) {
     set_error (EINVAL, "server does not support write operations");
@@ -422,10 +425,10 @@ 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,
-                                 void *opaque, callback_fn callback,
+                                 void *user_data, callback_fn callback,
                                  uint32_t flags)
 {
-  struct command_cb cb = { .opaque = opaque, .callback = callback, };
+  struct command_cb cb = { .user_data = user_data, .callback = callback, };
 
   /* Actually according to the NBD protocol document, servers do exist
    * that support NBD_CMD_CACHE but don't advertise the
@@ -456,10 +459,10 @@ 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,
-                                void *opaque, callback_fn callback,
+                                void *user_data, callback_fn callback,
                                 uint32_t flags)
 {
-  struct command_cb cb = { .opaque = opaque, .callback = callback, };
+  struct command_cb cb = { .user_data = user_data, .callback = callback, };
 
   if (nbd_unlocked_read_only (h) == 1) {
     set_error (EINVAL, "server does not support write operations");
@@ -489,20 +492,21 @@ 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,
-                               void *data, extent_fn extent,
+                               void *user_data, extent_fn extent,
                                uint32_t flags)
 {
-  return nbd_unlocked_aio_block_status_callback (h, count, offset, data,
extent,
+  return nbd_unlocked_aio_block_status_callback (h, count, offset,
+                                                 user_data, extent,
                                                  NULL, flags);
 }
 
 int64_t
 nbd_unlocked_aio_block_status_callback (struct nbd_handle *h,
                                         uint64_t count, uint64_t offset,
-                                        void *data, extent_fn extent,
+                                        void *user_data, extent_fn extent,
                                         callback_fn callback, uint32_t flags)
 {
-  struct command_cb cb = { .opaque = data, .fn.extent = extent,
+  struct command_cb cb = { .user_data = user_data, .fn.extent = extent,
                            .callback = callback, };
 
   if (!h->structured_replies) {
diff --git a/python/t/405-pread-structured.py b/python/t/405-pread-structured.py
index 9068a9a..49499b8 100644
--- a/python/t/405-pread-structured.py
+++ b/python/t/405-pread-structured.py
@@ -24,28 +24,30 @@ h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
 
 expected =
b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x08\x00\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00(\x00\x00\x00\x00\x00\x00\x000\x00\x00\x00\x00\x00\x00\x008\x00\x00\x00\x00\x00\x00\x00@\x00\x00\x00\x00\x00\x00\x00H\x00\x00\x00\x00\x00\x00\x00P\x00\x00\x00\x00\x00\x00\x00X\x00\x00\x00\x00\x00\x00\x00`\x00\x00\x00\x00\x00\x00\x00h\x00\x00\x00\x00\x00\x00\x00p\x00\x00\x00\x00\x00\x00\x00x\x00\x00\x00\x00\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x88\x00\x00\x00\x00\x00\x00\x00\x90\x00\x00\x00\x00\x00\x00\x00\x98\x00\x00\x00\x00\x00\x00\x00\xa0\x00\x00\x00\x00\x00\x00\x00\xa8\x00\x00\x00\x00\x00\x00\x00\xb0\x00\x00\x00\x00\x00\x00\x00\xb8\x00\x00\x00\x00\x00\x00\x00\xc0\x00\x00\x00\x00\x00\x00\x00\xc8\x00\x00\x00\x00\x00\x00\x00\xd0\x00\x00\x00\x00\x00\x00\x00\xd8\x00\x00\x00\x00\x00\x00\x00\xe0\x00\x00\x00\x00\x00\x00\x00\xe8\x00\x00\x00\x00\x00\x00\x00\xf0\x00\x00\x00\x00\x00\x00\x00\xf8\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x01\x08\x00\x00\x00\x00\x00\x00\x01\x10\x00\x00\x00\x00\x00\x00\x01\x18\x00\x00\x00\x00\x00\x00\x01
\x00\x00\x00\x00\x00\x00\x01(\x00\x00\x00\x00\x00\x00\x010\x00\x00\x00\x00\x00\x00\x018\x00\x00\x00\x00\x00\x00\x01@\x00\x00\x00\x00\x00\x00\x01H\x00\x00\x00\x00\x00\x00\x01P\x00\x00\x00\x00\x00\x00\x01X\x00\x00\x00\x00\x00\x00\x01`\x00\x00\x00\x00\x00\x00\x01h\x00\x00\x00\x00\x00\x00\x01p\x00\x00\x00\x00\x00\x00\x01x\x00\x00\x00\x00\x00\x00\x01\x80\x00\x00\x00\x00\x00\x00\x01\x88\x00\x00\x00\x00\x00\x00\x01\x90\x00\x00\x00\x00\x00\x00\x01\x98\x00\x00\x00\x00\x00\x00\x01\xa0\x00\x00\x00\x00\x00\x00\x01\xa8\x00\x00\x00\x00\x00\x00\x01\xb0\x00\x00\x00\x00\x00\x00\x01\xb8\x00\x00\x00\x00\x00\x00\x01\xc0\x00\x00\x00\x00\x00\x00\x01\xc8\x00\x00\x00\x00\x00\x00\x01\xd0\x00\x00\x00\x00\x00\x00\x01\xd8\x00\x00\x00\x00\x00\x00\x01\xe0\x00\x00\x00\x00\x00\x00\x01\xe8\x00\x00\x00\x00\x00\x00\x01\xf0\x00\x00\x00\x00\x00\x00\x01\xf8'
 
-def f (data, buf2, offset, s, err):
+def f (user_data, buf2, offset, s, err):
     assert err.value == 0
     err.value = errno.EPROTO
-    assert data == 42
+    assert user_data == 42
     assert buf2 == expected
     assert offset == 0
     assert s == nbd.READ_DATA
 
-buf = h.pread_structured (512, 0, 42, f)
+buf = h.pread_structured (512, 0, lambda *args: f (42, *args))
 
 print ("%r" % buf)
 
 assert buf == expected
 
-buf = h.pread_structured (512, 0, 42, f, nbd.CMD_FLAG_DF)
+buf = h.pread_structured (512, 0, lambda *args: f (42, *args),
+                          nbd.CMD_FLAG_DF)
 
 print ("%r" % buf)
 
 assert buf == expected
 
 try:
-    buf = h.pread_structured (512, 0, 43, f, nbd.CMD_FLAG_DF)
+    buf = h.pread_structured (512, 0, lambda *args: f (43, *args),
+                              nbd.CMD_FLAG_DF)
     assert False
 except nbd.Error as ex:
     assert ex.errnum == errno.EPROTO
diff --git a/python/t/460-block-status.py b/python/t/460-block-status.py
index 7e4ba2c..78c64f2 100644
--- a/python/t/460-block-status.py
+++ b/python/t/460-block-status.py
@@ -27,26 +27,27 @@ h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
                     "sh", script])
 
 entries = []
-def f (data, metacontext, offset, e, err):
+def f (user_data, metacontext, offset, e, err):
     global entries
-    assert data == 42
+    assert user_data == 42
     assert err.value == 0
     if metacontext != "base:allocation":
         return
     entries = e
 
-h.block_status (65536, 0, 42, f)
+h.block_status (65536, 0, lambda *args: f (42, *args))
 assert entries == [ 8192, 0,
                     8192, 1,
                    16384, 3,
                    16384, 2,
                    16384, 0]
 
-h.block_status (1024, 32256, 42, f)
+h.block_status (1024, 32256, lambda *args: f (42, *args))
 print ("entries = %r" % entries)
 assert entries == [  512, 3,
                    16384, 2]
 
-h.block_status (1024, 32256, 42, f, nbd.CMD_FLAG_REQ_ONE)
+h.block_status (1024, 32256, lambda *args: f (42, *args),
+                nbd.CMD_FLAG_REQ_ONE)
 print ("entries = %r" % entries)
 assert entries == [  512, 3]
diff --git a/python/t/600-debug-callback.py b/python/t/600-debug-callback.py
index 40cbd8f..cc03346 100644
--- a/python/t/600-debug-callback.py
+++ b/python/t/600-debug-callback.py
@@ -26,7 +26,7 @@ def f(id, context, msg):
     assert id == 42
     messages.append (msg)
 
-h.set_debug_callback (42, f)
+h.set_debug_callback (lambda *args: f (42, *args))
 
 h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "null"])
 h.shutdown ()
-- 
2.22.0

Eric Blake

2019-Jul-16 14:03 UTC

head link

Re: [Libguestfs] [PATCH libnbd v2] generator: Define new Closure type instead of callbacks.

On 7/16/19 6:04 AM, Richard W.M. Jones wrote:> A Closure is a list of (usually one, but can be more) closures.  In C
> there is also a singe ‘void *user_data’ parameter which is passed by
> the caller into the function and through as the first parameter of
> each callback invocation.
> 
> By grouping the previously separate Opaque and Callback* parameters
> together we can avoid the awkward situation where we have to scan
> through the argument list to try to match them up.
> 
> Because this is a closure, in non-C languages we can simply map these
> to a closure and drop the opaque parameter entirely.  It is not needed
> since languages with proper closures can capture local state in the
> closure.
> 
> Unlike the previous code it is no longer possible to mix persistent
> and non-persistent callbacks in the same API call.  This was not used
> before and is unlikely to be useful.
> 
> For the C API there is no API or ABI change (the only change is the
> naming of the opaque pointer which is not part of the API).  For the
> non-C languages the opaque parameter is no longer required as
> discussed above.
> 
> Partly based on Eric Blake's earlier work here:
> https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00160
> ---
> @@ -863,6 +864,10 @@ and arg >  | UInt of string           (* small
unsigned int *)
>  | UInt32 of string         (* 32 bit unsigned int *)
>  | UInt64 of string         (* 64 bit unsigned int *)
> +and closure = {
> +  cbname : string;         (* name of callback function *)
> +  cbargs : arg list;       (* all closures return int for now *)
Of course, now we could decide to make nbd_add_close_callback take a
closure that returns void, since we ignore the return value there.  But
that can be done on top, if at all.
> +}
>  and ret >  | RBool                    (* return a boolean, or error *)
>  | RConstString             (* return a const string, NULL for error *)
> @@ -915,9 +920,9 @@ Return the state of the debug flag on this
handle.";
>  
>    "set_debug_callback", {
>      default_call with
> -    args = [ Opaque "data";
> -             CallbackPersist ("debug_fn", [Opaque
"data";
> -                                           String "context";
String "msg"]) ];
> +    args = [ Closure (true,
> +                      [{ cbname="debug_fn";
> +                         cbargs=[String "context"; String
"msg"] }]) ];
One style of spacing (the list starts with '[{ ', and cbargs has no
space),
>      ret = RErr;
>      shortdesc = "set the debug callback";
>      longdesc = "\
> @@ -1345,10 +1350,11 @@ protocol extensions).";
>    "pread_structured", {
>      default_call with
>      args = [ BytesOut ("buf", "count"); UInt64
"offset";
> -             Opaque "data";
> -             Callback ("chunk", [ Opaque "data";
BytesIn ("subbuf", "count");
> +             Closure (false,
> +                      [{ cbname="chunk";
> +                         cbargs=[ BytesIn ("subbuf",
"count");
a second here (list start is same, cbargs now has a space after [)
>                                    UInt64 "offset"; Int
"status";
> -                                  Mutable (Int "error"); ]);
> +                                  Mutable (Int "error")] }]);
>               Flags "flags" ];
>      ret = RErr;
>      permitted_states = [ Connected ];
> @@ -1534,12 +1540,13 @@ punching a hole.";
>    "block_status", {
>      default_call with
>      args = [ UInt64 "count"; UInt64 "offset";
> -             Opaque "data";
> -             Callback ("extent", [Opaque "data";
String "metacontext";
> -                                  UInt64 "offset";
> -                                  ArrayAndLen (UInt32 "entries",
> -                                               "nr_entries");
> -                                  Mutable (Int "error")]);
> +             Closure (false,
> +                      [ {cbname="extent";
> +                         cbargs=[String "metacontext";
and a third here (list starts with '[ {', and cbargs has no space).
Probably worth picking a style you like and then using it consistently,
but that's cosmetic and doesn't affect the patch's operation.

> @@ -3143,19 +3151,19 @@ let generate_lib_libnbd_syms () >    pr " 
local: *;\n";
>    pr "};\n"
>  
> -let rec name_of_arg = function
> -| ArrayAndLen (arg, n) -> name_of_arg arg @ [n]
> +let rec c_name_of_arg = function
> +| ArrayAndLen (arg, n) -> c_name_of_arg arg @ [n]
>  | Bool n -> [n]
>  | BytesIn (n, len) -> [n; len]
>  | BytesOut (n, len) -> [n; len]
>  | BytesPersistIn (n, len) -> [n; len]
>  | BytesPersistOut (n, len) -> [n; len]
> -| Callback (n, _) | CallbackPersist (n, _) -> [n]
> +| Closure (_, closures) ->
> +   "user_data" :: List.map (fun { cbname } -> cbname)
closures
This hard-coded parameter name (in C) implies that you can have at most
one Closure in an arg list. Should we enforce that (the same way we
enforce that there is at most one Flags)?

> @@ -3164,13 +3172,18 @@ let rec name_of_arg = function
>  | UInt32 n -> [n]
>  | UInt64 n -> [n]
>  
> -let rec print_c_arg_list ?(handle = false) args > +let rec
print_c_arg_list ?(handle = false) ?(user_data = false) args >    pr
"(";
>    let comma = ref false in
>    if handle then (
>      comma := true;
>      pr "struct nbd_handle *h";
>    );
> +  if user_data then (
> +    if !comma then pr ", ";
> +    comma := true;
> +    pr "void *user_data";
> +  );
A bit different from my approach, but it works. Less change to existing
callers, at any rate.

> @@ -3560,9 +3577,9 @@ are called is not defined.  This API is only
available
>  from C and is designed to help when writing bindings to libnbd
>  from other programming languages.
>  
> - typedef void (*nbd_close_callback) (void *data);
> + typedef void (*nbd_close_callback) (void *user_data);
I might have split out the rename churn (s/\(data\|opaque\)/user_data/)
in a separate patch; but it's probably not worth it now.

>  let print_python_binding name { args; ret } > -  (* Functions with a
callback parameter are special because we
> -   * have to generate a wrapper function which translates the
> -   * callback parameters back to Python.
> +  (* Functions with a Closure parameter are special because we
> +   * have to generate wrapper functions which translate the
> +   * callbacks back to Python.
>     *)
> -       pr "\n";
> +       (* Persistent closures need an explicit function to decrement
> +        * the closure refcounts and free the user_data struct.
> +        *)
> +       if persistent then (
> +         pr "static void\n";
> +         pr "free_%s_user_data (void *vp)\n" name;
> +         pr "{\n";
> +         pr "  struct %s_user_data *user_data = vp;\n" name;
> +         pr "\n";
> +         List.iter (
> +           fun { cbname } ->
> +             pr "  Py_DECREF (user_data->%s);\n" cbname
Oh, good fix. We were not previously incrementing the ref-count of the
Python Callable, and could have ended up deferencing a garbage-collected
object.
> +         ) cls;
> +         pr "  free (user_data);\n";
> +         pr "}\n";
> +         pr "\n";
> +       );
> +
>      | Flags n ->
>         pr "  uint32_t %s_u32;\n" n;
>         pr "  unsigned int %s; /* really uint32_t */\n" n
> @@ -3911,8 +3930,7 @@ let print_python_binding name { args; ret } >     
pr "  int64_t %s_i64;\n" n;
>         pr "  long long %s; /* really int64_t */\n" n
>      | Mutable arg ->
> -       pr "  PyObject *%s;\n" (List.hd (name_of_arg arg))
> -    | Opaque _ -> ()
> +       pr "  PyObject *%s;\n" (List.hd (c_name_of_arg arg))
I might have also split out the patch for the rename of s/name_of_arg/c_&/
>      | Path n ->
>         pr "  PyObject *py_%s = NULL;\n" n;
>         pr "  char *%s = NULL;\n" n
> @@ -3935,34 +3953,17 @@ let print_python_binding name { args; ret } >   
) args;
>    pr "\n";
>  
> -  (* Allocate the parameter. *)
> +  (* Allocate the persistent closure user_data. *)
>    List.iter (
>      function
> -    | ArrayAndLen _
> -    | Bool _
> -    | BytesIn _
> -    | BytesPersistIn _
> -    | BytesOut _
> -    | BytesPersistOut _
> -    | Callback _ -> ()
> -    | CallbackPersist (n, _) ->
> -       pr "  %s_data = malloc (sizeof *%s_data);\n" n n;
> -       pr "  if (%s_data == NULL) {\n" n;
> +    | Closure (false, _) -> ()
> +    | Closure (true, cls) ->
> +       pr "  user_data = malloc (sizeof *user_data);\n";
> +       pr "  if (user_data == NULL) {\n";
>         pr "    PyErr_NoMemory ();\n";
>         pr "    return NULL;\n";
>         pr "  }\n"
Memory leak. If Closure contains two callbacks, and the first malloc()
succeeds while the second fails, we are not reclaiming the first.
Should our generated python code take advantage of
__attribute__((cleanup)) to make it easier to write?
> @@ -4017,6 +4017,20 @@ let print_python_binding name { args; ret } >   
pr "))\n";
>    pr "    return NULL;\n";
>  
> +  (* If the parameter has persistent closures then we need to
> +   * make sure the ref count remains positive.
> +   *)
> +  List.iter (
> +    function
> +    | Closure (false, _) -> ()
> +    | Closure (true, cls) ->
> +       List.iter (
> +         fun { cbname } ->
> +           pr "  Py_INCREF (user_data->%s);\n" cbname
> +       ) cls
> +    | _ -> ()
> +  ) args;
> +
Any reason this loop is a separate pass, rather than...
>    pr "  h = get_handle (py_h);\n";
>    List.iter (
>      function
> @@ -4044,19 +4058,20 @@ let print_python_binding name { args; ret } >   
pr "  %s = malloc (%s);\n" n count
>      | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
>         pr "  %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n
n
> -    | Callback (n, _) | CallbackPersist (n, _) ->
> -       pr "  if (!PyCallable_Check (%s_data->fn)) {\n" n;
> -       pr "    PyErr_SetString (PyExc_TypeError,\n";
> -       pr "                     \"callback parameter %s is not
callable\");\n"
> -          n;
> -       pr "    return NULL;\n";
> -       pr "  }\n"
> +    | Closure (_, cls) ->
> +       List.iter (
> +         fun { cbname } ->
> +           pr "  if (!PyCallable_Check (user_data->%s)) {\n"
cbname;
> +           pr "    PyErr_SetString (PyExc_TypeError,\n";
> +           pr "                     \"callback parameter %s is
not callable\");\n" cbname;
> +           pr "    return NULL;\n";
> +           pr "  }\n"
> +       ) cls
...done here?  Also, you have a reference leaks: if the python code
passes a non-Callable, you fail to DECREF it (made trickier when the
Closure has two callbacks, and only the second callback is not Callable).

> @@ -4302,28 +4319,30 @@ class NBD (object):
>  
>    List.iter (
>      fun (name, { args; shortdesc; longdesc }) ->
> -      let args = List.map (
> -                     function
> -                     | ArrayAndLen (UInt32 n, _) -> n, None
> -                     | ArrayAndLen _ -> assert false
> -                     | UInt32 n -> n, None
> -                     | UInt64 n -> n, None
> -                   ) args in
> +      let args > +        List.map (
> +            function
> +            | ArrayAndLen (UInt32 n, _) -> [n, None]
> +            | ArrayAndLen _ -> assert false
> +            | Bool n -> [n, None]
> +            | BytesIn (n, _) | BytesPersistIn (n, _) -> [n, None]
> +            | BytesPersistOut (n, _) -> [n, None]
> +            | BytesOut (_, count) -> [count, None]
> +            | Closure (_, cls) -> 
> +               List.map (fun { cbname } -> cbname, None) cls
Nicer than my attempt.

> +++ b/generator/states-reply-simple.c
> @@ -64,7 +64,7 @@
>        int error = 0;
>  
>        assert (cmd->error == 0);
> -      if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data,
cmd->count,
> +      if (cmd->cb.fn.read (cmd->cb.user_data, cmd->data,
cmd->count,
And here's an example of the rename churn that might be worth a separate
patch.

> +++ b/lib/internal.h
> @@ -212,7 +212,7 @@ struct meta_context {
>  struct close_callback {
>    struct close_callback *next;  /* Linked list. */
>    nbd_close_callback cb;        /* Function. */
> -  void *data;                   /* Data. */
> +  void *user_data;              /* Data. */
>  };
>  
>  struct socket_ops {
> @@ -241,14 +241,15 @@ struct socket {
>    const struct socket_ops *ops;
>  };
>  
> -typedef int (*extent_fn) (void *data, const char *metacontext, uint64_t
offset,
> +typedef int (*extent_fn) (void *user_data,
> +                          const char *metacontext, uint64_t offset,
>                            uint32_t *entries, size_t nr_entries, int
*error);
> -typedef int (*read_fn) (void *data, const void *buf, size_t count,
> +typedef int (*read_fn) (void *user_data, const void *buf, size_t count,
>                          uint64_t offset, int status, int *error);
> -typedef int (*callback_fn) (void *data, int64_t cookie, int *error);
> +typedef int (*callback_fn) (void *user_data, int64_t cookie, int *error);
>  
>  struct command_cb {
> -  void *opaque;
> +  void *user_data;
since none of the renames here are necessitated by the generator
changes, but rather make things more consistent.
> +++ b/python/t/405-pread-structured.py
> @@ -24,28 +24,30 @@ h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
>  
>  
> -def f (data, buf2, offset, s, err):
> +def f (user_data, buf2, offset, s, err):
>      assert err.value == 0
>      err.value = errno.EPROTO
> -    assert data == 42
> +    assert user_data == 42
>      assert buf2 == expected
>      assert offset == 0
>      assert s == nbd.READ_DATA
>  
> -buf = h.pread_structured (512, 0, 42, f)
> +buf = h.pread_structured (512, 0, lambda *args: f (42, *args))
Nice.

We still need to add coverage of h.aio_pread_structured_callback, to
prove that this actually did what we wanted when two callbacks are
present (the test will fail without this patch).

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

Maybe Matching Threads

Search for more apparently analagous threads

Libguestfs - Jul 2019 - Re: [PATCH libnbd v2] generator: Define new Closure type instead of callbacks.

[Libguestfs] [PATCH libnbd v2] generator: Define new Closure type

[Libguestfs] [PATCH libnbd v2] generator: Define new Closure type instead of callbacks.

Re: [Libguestfs] [PATCH libnbd v2] generator: Define new Closure type instead of callbacks.

Maybe Matching Threads