Libguestfs - Jul 2019 - [RFC libnbd PATCH 0/2] Start fixing python nbd.pread_structured_callback

Posting now that I got something to compile (at the expense of
breaking OCaml bindings), but I'm open to ideas on how to improve it.

Eric Blake (2):
  generator: Tweak print_c_arg_list to take alternate first arg
  RFC: generator: Handle shared callbacks in Python

 generator/generator | 556 ++++++++++++++++++++++----------------------
 1 file changed, 280 insertions(+), 276 deletions(-)

-- 
2.20.1

The next patch plans to refactor callbacks to make it possible to
share the same opaque data, but to do so, we want to generate more
than one function with the same canned first argument, different from
our top-level function first argument of struct nbd_handle.  Update
the ~handle argument to be 'string option'.
---
 generator/generator | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/generator/generator b/generator/generator
index caa6353..ad12b36 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3164,12 +3164,14 @@ 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 = None) args    pr "(";
   let comma = ref false in
-  if handle then (
-    comma := true;
-    pr "struct nbd_handle *h";
+  (match handle with
+   | Some arg ->
+      comma := true;
+      pr "%s" arg
+   | None -> ()
   );
   List.iter (
     fun arg ->
@@ -3214,7 +3216,7 @@ let print_call name args ret     | RString -> pr
"char *"
   );
   pr "nbd_%s " name;
-  print_c_arg_list ~handle:true args
+  print_c_arg_list ~handle:(Some "struct nbd_handle *h") args

 let print_extern name args ret    pr "extern ";
@@ -3360,7 +3362,7 @@ let generate_lib_api_c () 
     pr "%s\n" ret_c_type;
     pr "nbd_%s " name;
-    print_c_arg_list ~handle:true args;
+    print_c_arg_list ~handle:(Some "struct nbd_handle *h") args;
     pr "\n";
     pr "{\n";
     (match ret with
-- 
2.20.1

[RFC because generated OCaml code needs the same treatment, but I ran
out of time to play with that. At least 'make -C python check' passes,
although coverage is not complete yet, as there is no
python/t/5XX-pread-structured-callback.py...]

[Also RFC because I'm not sure if the use of a record type for
'callback' is the best approach or most idiomatic OCaml - but hey, it
taught me a lot about OCaml. Writing a 'string * callback list' proved
to be a rather interesting exercise]

Our C code has a couple of functions that want to take multiple
callback functions that share a single opaque data
(nbd_aio_pread_structured_callback; nbd_aio_block_status_callback).
However, the mapping of this construct to Python passed only the
opaque handle of the first function to the second callback wrapper,
which means we would invoke the wrong Python Callable; better is
tracking a single malloc()d structure containing the Python opaque
object, and ALL of the Python Callables that reuse that same opaque
object.

We could either add lots of static checking that a
Callback/CallbackPersist cannot appear in the argument list until
after an earlier Opaque, and that the argument name for top-level
Opaque also appears as the first argument for the callback function.
Or, we could bundle the construct through a better constructor that
tracks that an opaque argument and one or more callback functions must
be handled as a unit, where the callback function automatically
outputs the opaque argument first.  This patch attempts the latter.

It may also be worth comparing a diff of the generated
python/methods.c before and after this patch, to see what changes.
---
 generator/generator | 542 ++++++++++++++++++++++----------------------
 1 file changed, 272 insertions(+), 270 deletions(-)

diff --git a/generator/generator b/generator/generator
index ad12b36..e48b1ad 100755
--- a/generator/generator
+++ b/generator/generator
@@ -841,6 +841,13 @@ type call = {
    *)
   may_set_error : bool;
 }
+and callback = {
+  name : string;           (* callback name *)
+  cbargs : arg list;       (* parameters (except opaque) *)
+  (* For now, all callbacks return int; we could list a return type here if
+   * more variety is needed.
+   *)
+}
 and arg  | ArrayAndLen of arg * string (* array + number of entries *)
 | Bool of string           (* bool *)
@@ -849,13 +856,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 *)
 | 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 *)
+| OpaqueAndCallbacks of string * callback list (* one or more callbacks
+                                                * sharing same opaque *)
+| OpaqueAndCallbacksPersist of string * callback list (* as above, but opaque
+                                                       * persists *)
 | Path of string           (* filename or path *)
 | SockAddrAndLen of string * string (* struct sockaddr * + socklen_t *)
 | String of string         (* string *)
@@ -915,9 +923,10 @@ 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 = [ OpaqueAndCallbacksPersist ("data",
+                                        [ {name="debug_fn";
+                                           cbargs=[ String "context";
+                                                    String "msg" ]}])
];
     ret = RErr;
     shortdesc = "set the debug callback";
     longdesc = "\
@@ -1345,10 +1354,11 @@ protocol extensions).";
   "pread_structured", {
     default_call with
     args = [ BytesOut ("buf", "count"); UInt64
"offset";
-             Opaque "data";
-             Callback ("chunk", [ Opaque "data"; BytesIn
("subbuf", "count");
-                                  UInt64 "offset"; Int
"status";
-                                  Mutable (Int "error"); ]);
+             OpaqueAndCallbacks ("data",
+                                 [ {name="chunk";
+                                    cbargs=[ BytesIn ("subbuf",
"count");
+                                             UInt64 "offset"; Int
"status";
+                                             Mutable (Int "error")
]}]);
              Flags "flags" ];
     ret = RErr;
     permitted_states = [ Connected ];
@@ -1534,12 +1544,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")]);
+             OpaqueAndCallbacks ("data",
+                                 [ {name="extent";
+                                    cbargs=[String "metacontext";
+                                            UInt64 "offset";
+                                            ArrayAndLen (UInt32
"entries",
+                                                        
"nr_entries");
+                                            Mutable (Int "error")
]}]);
              Flags "flags" ];
     ret = RErr;
     permitted_states = [ Connected ];
@@ -1717,9 +1728,10 @@ 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") ]);
+             OpaqueAndCallbacksPersist ("data",
+                                        [ {name="callback";
+                                           cbargs=[ Int64 "cookie";
+                                                    Mutable (Int
"error") ]}]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1747,12 +1759,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"); ]);
+             OpaqueAndCallbacksPersist ("data",
+                                        [ {name="chunk";
+                                           cbargs=[ BytesIn
("subbuf", "count");
+                                                    UInt64 "offset";
+                                                    Int "status";
+                                                    Mutable (Int
"error"); ]}]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1769,14 +1781,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") ]);
+             OpaqueAndCallbacksPersist ("data",
+                                        [ {name="chunk";
+                                           cbargs=[ BytesIn
("subbuf", "count");
+                                                    UInt64 "offset";
+                                                    Int "status";
+                                                    Mutable (Int
"error"); ]};
+                                          {name="callback";
+                                           cbargs=[ Int64 "cookie";
+                                                    Mutable (Int
"error") ]}]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1819,9 +1832,10 @@ 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") ]);
+             OpaqueAndCallbacksPersist ("data",
+                                        [ {name="callback";
+                                           cbargs=[ Int64 "cookie";
+                                                    Mutable (Int
"error") ]}]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1884,9 +1898,10 @@ 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 = [ OpaqueAndCallbacksPersist ("data",
+                                        [ {name="callback";
+                                           cbargs=[ Int64 "cookie";
+                                                    Mutable (Int
"error") ]}]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1927,9 +1942,10 @@ 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") ]);
+             OpaqueAndCallbacksPersist ("data",
+                                        [ {name="callback";
+                                           cbargs=[ Int64 "cookie";
+                                                    Mutable (Int
"error") ]}]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1970,9 +1986,10 @@ 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") ]);
+             OpaqueAndCallbacksPersist ("data",
+                                        [ {name="callback";
+                                           cbargs=[ Int64 "cookie";
+                                                    Mutable (Int
"error") ]}]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2013,9 +2030,10 @@ 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") ]);
+             OpaqueAndCallbacksPersist ("data",
+                                        [ {name="callback";
+                                           cbargs=[ Int64 "cookie";
+                                                    Mutable (Int
"error") ]}]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2042,12 +2060,13 @@ 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")]);
+             OpaqueAndCallbacksPersist ("data",
+                                        [ {name="extent";
+                                           cbargs=[ String
"metacontext";
+                                                    UInt64 "offset";
+                                                    ArrayAndLen (UInt32
"entries",
+                                                                
"nr_entries");
+                                                    Mutable (Int
"error") ]}]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2063,14 +2082,16 @@ 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") ]);
+             OpaqueAndCallbacksPersist ("data",
+                                        [ {name="extent";
+                                           cbargs=[ String
"metacontext";
+                                                    UInt64 "offset";
+                                                    ArrayAndLen (UInt32
"entries",
+                                                                
"nr_entries");
+                                                    Mutable (Int
"error") ]};
+                                          {name="callback";
+                                           cbargs=[ Int64 "cookie";
+                                                    Mutable (Int
"error") ]}]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -3150,11 +3171,11 @@ let rec name_of_arg = function
 | BytesOut (n, len) -> [n; len]
 | BytesPersistIn (n, len) -> [n; len]
 | BytesPersistOut (n, len) -> [n; len]
-| Callback (n, _) | CallbackPersist (n, _) -> [n]
 | Flags n -> [n]
 | Int n -> [n]
 | Int64 n -> [n]
-| Opaque n -> [n]
+| OpaqueAndCallbacks (n, cbs) | OpaqueAndCallbacksPersist (n, cbs) ->
+   [n] @ List.map (fun cb -> cb.name) cbs
 | Mutable arg -> name_of_arg arg
 | Path n -> [n]
 | SockAddrAndLen (n, len) -> [n; len]
@@ -3185,15 +3206,17 @@ let rec print_c_arg_list ?(handle = None) 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
       | 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
+      | OpaqueAndCallbacks (n, cbs) | OpaqueAndCallbacksPersist (n, cbs) ->
+         pr "void *%s" n;
+         List.iter (fun cb ->
+           let first = "void *" ^ n in
+           pr ", int (*%s)" cb.name;
+           print_c_arg_list ~handle:(Some first) cb.cbargs) cbs
       | Path n
       | String n -> pr "const char *%s" n
       | StringList n -> pr "char **%s" n
@@ -3715,155 +3738,138 @@ let print_python_binding name { args; ret }     * have
to generate a wrapper function which translates the
    * callback parameters 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
+  let print_callback cb +    pr "static int\n";
+    pr "%s_%s_wrapper " name cb.name;
+    print_c_arg_list ~handle:(Some "void *_data") cb.cbargs;
+    pr "\n";
+    pr "{\n";
+    pr "  int ret;\n";
+    pr "  PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
+    pr "  PyObject *py_args, *py_ret;\n";
+    pr "  struct %s_data *data = _data;\n" name;
+    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 _
+      | Flags _ | Mutable _
+      | OpaqueAndCallbacks _ | OpaqueAndCallbacksPersist _
+      | Path _ | SockAddrAndLen _ | StringList _
+      | UInt _ | UInt32 _ -> assert false
+    ) cb.cbargs;
+    pr "\n";
+
+    pr "  py_args = Py_BuildValue (\"(O\"";
+    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 _
+      | Flags _ | Mutable _
+      | OpaqueAndCallbacks _ | OpaqueAndCallbacksPersist _
+      | Path _ | SockAddrAndLen _ | StringList _
+      | UInt _ | UInt32 _ -> assert false
+    ) cb.cbargs;
+    pr " \")\", data->data";
+    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 _
+      | Flags _ | Mutable _
+      | OpaqueAndCallbacks _ | OpaqueAndCallbacksPersist _
+      | Path _ | SockAddrAndLen _ | StringList _
+      | UInt _ | UInt32 _ -> assert false
+    ) cb.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 (data->%s, py_args);\n"
cb.name;
+    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 _
+      | Flags _ | Mutable _
+      | OpaqueAndCallbacks _ | OpaqueAndCallbacksPersist _
+      | Path _ | SockAddrAndLen _ | StringList _
+      | UInt _ | UInt32 _ -> assert false
+    ) cb.cbargs;
+    pr "  return ret;\n";
+    pr "}\n";
+    pr "\n"
   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";
+    | OpaqueAndCallbacks (data, cbs) | OpaqueAndCallbacksPersist (data, cbs)
->
+       pr "struct %s_data {\n" name;
        pr "  PyObject *data;\n";
+       List.iter (fun cb ->
+         pr "  PyObject *%s;\n" cb.name) cbs;
        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";
-       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"
+       List.iter (fun cb -> print_callback cb) cbs
     | _ -> ()
   ) args;

@@ -3901,10 +3907,6 @@ 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
     | Flags n ->
        pr "  uint32_t %s_u32;\n" n;
        pr "  unsigned int %s; /* really uint32_t */\n" n
@@ -3914,7 +3916,10 @@ let print_python_binding name { args; ret }         pr
"  long long %s; /* really int64_t */\n" n
     | Mutable arg ->
        pr "  PyObject *%s;\n" (List.hd (name_of_arg arg))
-    | Opaque _ -> ()
+    | OpaqueAndCallbacks (n, _) ->
+       pr "  struct %s_data _%s, *%s = &_%s;\n" name n n n
+    | OpaqueAndCallbacksPersist (n, _) ->
+       pr "  struct %s_data *%s;\n" name n
     | Path n ->
        pr "  PyObject *py_%s = NULL;\n" n;
        pr "  char *%s = NULL;\n" n
@@ -3946,17 +3951,16 @@ let print_python_binding name { args; ret }      |
BytesPersistIn _
     | BytesOut _
     | BytesPersistOut _
-    | Callback _ -> ()
-    | CallbackPersist (n, _) ->
-       pr "  %s_data = malloc (sizeof *%s_data);\n" n n;
-       pr "  if (%s_data == NULL) {\n" n;
-       pr "    PyErr_NoMemory ();\n";
-       pr "    return NULL;\n";
-       pr "  }\n"
     | Flags _
     | Int _
     | Int64 _
-    | Opaque _
+    | OpaqueAndCallbacks _ -> ()
+    | OpaqueAndCallbacksPersist (n, _) ->
+       pr "  %s = malloc (sizeof *%s);\n" n n;
+       pr "  if (%s == NULL) {\n" n;
+       pr "    PyErr_NoMemory ();\n";
+       pr "    return NULL;\n";
+       pr "  }\n"
     | Mutable _
     | Path _
     | SockAddrAndLen _
@@ -3977,12 +3981,13 @@ 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\""
     | Flags n -> pr " \"I\""
     | Int n -> pr " \"i\""
     | Int64 n -> pr " \"L\""
     | Mutable _ -> pr " \"O\""
-    | Opaque _ -> pr " \"O\""
+    | OpaqueAndCallbacks (n, cbs) | OpaqueAndCallbacksPersist (n, cbs) ->
+       pr " \"O\"";
+       List.iter (fun _ -> pr " \"O\"") cbs
     | Path n -> pr " \"O&\""
     | SockAddrAndLen (n, _) -> pr " \"O\""
     | String n -> pr " \"s\""
@@ -4002,12 +4007,13 @@ 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
     | 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)
+    | OpaqueAndCallbacks (n, cbs) | OpaqueAndCallbacksPersist (n, cbs) ->
+       pr ", &%s->data" n;
+       List.iter (fun cb -> pr ", &%s->%s" n cb.name) cbs
     | Path n -> pr ", PyUnicode_FSConverter, &py_%s" n
     | SockAddrAndLen (n, _) -> pr ", &%s" n
     | String n -> pr ", &%s" n
@@ -4046,19 +4052,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"
     | 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 -> ()
+    | OpaqueAndCallbacks (n, cbs) | OpaqueAndCallbacksPersist (n, cbs) ->
+       List.iter (fun cb ->
+         pr "  if (!PyCallable_Check (%s->%s)) {\n" n cb.name;
+         pr "    PyErr_SetString (PyExc_TypeError,\n";
+         pr "                     \"callback parameter %s is not
callable\");\n"
+            cb.name;
+         pr "    return NULL;\n";
+         pr "  }\n"
+       ) cbs
     | Path n ->
        pr "  %s = PyBytes_AS_STRING (py_%s);\n" n n;
        pr "  assert (%s != NULL);\n" n
@@ -4084,12 +4091,13 @@ 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
     | 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)
+    | OpaqueAndCallbacks (n, cbs) | OpaqueAndCallbacksPersist (n, cbs) ->
+       pr ", %s" n;
+       List.iter (fun cb -> pr ", %s_%s_wrapper" name cb.name) cbs
     | Path n -> pr ", %s" n
     | SockAddrAndLen (n, _) -> pr ", /* XXX */ (void *) %s, 0" n
     | String n -> pr ", %s" n
@@ -4119,12 +4127,11 @@ let print_python_binding name { args; ret }      | Bool
_
     | BytesIn _
     | BytesPersistIn _ | BytesPersistOut _
-    | Callback _ | CallbackPersist _
     | Flags _
     | Int _
     | Int64 _
     | Mutable _
-    | Opaque _
+    | OpaqueAndCallbacks _ | OpaqueAndCallbacksPersist _
     | Path _
     | SockAddrAndLen _
     | String _
@@ -4162,15 +4169,14 @@ let print_python_binding name { args; ret }      | Bool
_ -> ()
     | BytesIn (n, _) -> pr "  PyBuffer_Release (&%s);\n" n
     | BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> ()
-    | Callback _ -> ()
-    | CallbackPersist (n, _) ->
-       pr "  /* This ensures the callback data is freed eventually.
*/\n";
-       pr "  nbd_add_close_callback (h, free, %s_data);\n" n
     | Flags _ -> ()
     | Int _ -> ()
     | Int64 _ -> ()
     | Mutable _ -> ()
-    | Opaque _ -> ()
+    | OpaqueAndCallbacks _ -> ()
+    | OpaqueAndCallbacksPersist (n, _) ->
+       pr "  /* This ensures the callback data is freed eventually.
*/\n";
+       pr "  nbd_add_close_callback (h, free, %s);\n" n
     | Path n ->
        pr "  Py_XDECREF (py_%s);\n" n
     | SockAddrAndLen _ -> ()
@@ -4312,12 +4318,13 @@ class NBD (object):
                      | 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
+                     | OpaqueAndCallbacks (n, cbs)
+                     | OpaqueAndCallbacksPersist (n, cbs) ->
+                        n ^ ", " ^ String.concat ", "
(List.map (fun cb -> cb.name) cbs), None
                      | Path n -> n, None
                      | SockAddrAndLen (n, _) -> n, None
                      | String n -> n, None
@@ -4399,16 +4406,12 @@ and ocaml_arg_to_string = function
   | OCamlArg (BytesPersistIn _) -> "Buffer.t"
   | OCamlArg (BytesOut _) -> "bytes"
   | OCamlArg (BytesPersistOut _) -> "Buffer.t"
-  | OCamlArg (Callback (_, args))
-  | OCamlArg (CallbackPersist (_, args)) ->
-     sprintf "(%s)"
-             (ocaml_fundecl_to_string (List.map (fun a -> OCamlArg a) args)
-                                      RErr)
   | OCamlArg (Flags _) -> assert false (* see above *)
   | OCamlArg (Int _) -> "int"
   | OCamlArg (Int64 _) -> "int64"
   | OCamlArg (Mutable arg) -> ocaml_arg_to_string (OCamlArg arg) ^ "
ref"
-  | OCamlArg (Opaque n) -> sprintf "'%s" n
+  | OCamlArg (OpaqueAndCallbacks _) | OCamlArg (OpaqueAndCallbacksPersist _)
->
+     sprintf "string" (* XXX not impl *)
   | OCamlArg (Path _) -> "string"
   | OCamlArg (SockAddrAndLen _) -> "string" (* XXX not impl *)
   | OCamlArg (String _) -> "string"
@@ -4574,22 +4577,13 @@ external close : t -> unit =
\"nbd_internal_ocaml_nbd_close\"

 let print_ocaml_binding (name, { args; ret })    (* Functions with a callback
parameter require special handling. *)
-  let find_callback opaque_id -    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

   List.iter (
     function
+    | OpaqueAndCallbacks (cb_name, cbs)
+    | OpaqueAndCallbacksPersist (cb_name, cbs) ->
+       pr " /* Callbacks not implemented yet... */\n"
+      (*
     | Callback (cb_name, args) | CallbackPersist (cb_name, args) ->
        let argnames           List.map (
@@ -4642,7 +4636,7 @@ let print_ocaml_binding (name, { args; ret })             
pr "  %sv = *_%s->data;\n" n n
          | Mutable (Int n) ->
             pr "  %sv = caml_alloc_tuple (1);\n" n;
-            pr "  Store_field (%sv, 0, Val_int (*%s));\n" n n
+            pr "  Store_field (%sv, 0, Val_int (" ^
"*%s));\n" n n
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
          | BytesPersistIn _ | BytesPersistOut _
@@ -4694,7 +4688,8 @@ let print_ocaml_binding (name, { args; ret })         pr
"  caml_enter_blocking_section ();\n";
        pr "  return ret;\n";
        pr "}\n"
-    | _ -> ()
+       *)
+     | _ -> ()
   ) args;

   (* Convert the generic args to OCaml args. *)
@@ -4760,6 +4755,7 @@ let print_ocaml_binding (name, { args; ret })         pr
"  struct nbd_buffer %s_buf = NBD_buffer_val (%sv);\n" n n;
        pr "  void *%s = %s_buf.data;\n" n n;
        pr "  size_t %s = %s_buf.len;\n" count n
+       (*
     | OCamlArg (Callback (n, _)) ->
        pr "  /* This is safe because we only call this while this
stack\n";
        pr "   * frame is live.\n";
@@ -4777,12 +4773,14 @@ let print_ocaml_binding (name, { args; ret })         pr
"  nbd_add_close_callback (h, nbd_internal_ocaml_free_root, %s_cb);\n"
           n;
        pr "  const void *%s = %s_%s_wrapper;\n" n name n
+        *)
     | OCamlArg (Flags _) -> assert false (* see above *)
     | OCamlArg (Int n) ->
        pr "  int %s = Int_val (%sv);\n" n n
     | OCamlArg (Int64 n) ->
        pr "  int64_t %s = Int64_val (%sv);\n" n n
     | OCamlArg (Mutable _) -> assert false
+      (*
     | OCamlArg (Opaque n) ->
        (match find_callback n with
         | Callback (cb_name, _) ->
@@ -4801,6 +4799,9 @@ let print_ocaml_binding (name, { args; ret })            
pr "  nbd_add_close_callback (h, nbd_internal_ocaml_free_root,
%s_data);\n" n
         | _ -> assert false
        )
+       *)
+    | OCamlArg (OpaqueAndCallbacks _) | OCamlArg (OpaqueAndCallbacksPersist _)
->
+       pr "  /* callbacks not implemented yet */\n"
     | OCamlArg (Path n) | OCamlArg (String n) ->
        pr "  const char *%s = String_val (%sv);\n" n n
     | OCamlArg (SockAddrAndLen (n, len)) ->
@@ -4818,6 +4819,7 @@ let print_ocaml_binding (name, { args; ret })    ) oargs;

   (* Create wrapper data for callbacks. *)
+  (*
   List.iter (
     function
     | OCamlArg (Opaque n) ->
@@ -4836,6 +4838,7 @@ let print_ocaml_binding (name, { args; ret })         )
     | _ -> ()
   ) oargs;
+   *)

   let errcode      match ret with
@@ -4877,13 +4880,12 @@ let print_ocaml_binding (name, { args; ret })      |
OCamlArg (BytesPersistIn _)
     | OCamlArg (BytesOut _)
     | OCamlArg (BytesPersistOut _)
-    | OCamlArg (Callback _)
-    | OCamlArg (CallbackPersist _)
     | OCamlArg (Flags _)
     | OCamlArg (Int _)
     | OCamlArg (Int64 _)
     | OCamlArg (Mutable _)
-    | OCamlArg (Opaque _)
+    | OCamlArg (OpaqueAndCallbacks _)
+    | OCamlArg (OpaqueAndCallbacksPersist _)
     | OCamlArg (Path _)
     | OCamlArg (String _)
     | OCamlArg (SockAddrAndLen _)
-- 
2.20.1

I have a partial patch which fixes this in a slightly different (but
overall quite similar) way.  The key observations are:

(1) OpaqueAndCallbacks is otherwise known as a list of Closures.  A
closure list in C corresponds to an opaque pointer + several function
pointers (as in your patch).

(2) But in other languages, Closures capture local state already so
don't require the opaque pointer at all.  ie. In OCaml you can already
write this to capture some local_data in the closure:

  let my_chunk_cb local_data subbuf count offset stats error     ...
  in

  NBD.pread_structured buf count offset (my_chunk_cb local_data) flags

Similarly in Python you can use lambda.  There's no need for the
opaque to be explicit at all in non-C languages.

(3) I don't think we actually have to name the opaque pointer.  We can
just always call it "void *user_data" in C.  (There is max one opaque
pointer per function).  This simplifies things a bit.

(4) We can also probably assume that either all non-persistent or all
persistent callbacks are required.  (Your patch does that already, but
mine uses a boolean flag instead of separate case).

Will post later.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html