thr3ads.net - Libguestfs - [Libguestfs] [PATCH libnbd 1/6] generator: Share single list of all Closures. [Aug 2019]

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

Richard W.M. Jones

2019-Aug-13 10:06 UTC

[Libguestfs] [PATCH 0/6] Implement OClosure.

Patches 1-4 are basically uncontroversial, straightforward refactoring
and IMHO we should just push them.  Possibly 1-3 should be squashed
together, but I posted them separately so they are easier to review.

Patches 5 and 6 together implement OClosure.  Patch 5 adds the feature
and is simple to understand.

Patch 6 changes the Closure completion callbacks into OClosure, but
because it doesn't change the position within the list of arguments
the C API doesn't change at all.  The Python API also doesn't change
for the same reason.  (The OCaml API _does_ change but not
significantly).

I've run out of time on this patch today because I've got meetings for
the rest of the day (and it's only 11am!)  However if I did have more
time there would be a 7th patch in the series which renames all
aio_*_callback functions to simply aio_*.  eg. nbd_aio_pread_callback
is renamed to nbd_aio_pread (and old nbd_aio_pread is removed).  That
is obviously a somewhat large albeit it mechanical API change.

Rich.

Richard W.M. Jones

2019-Aug-13 10:06 UTC

head link

[Libguestfs] [PATCH libnbd 1/6] generator: Share single list of all Closures.

This change does not affect the output of the generator.

Note this requires that all Closure args have the same parameter name
whichever method they appear in.  An alternate refactoring could work
the same way as Flags and Enum where the parameter name is part of the
arg, eg:

  type arg   ...
  | Closure of string * closure   (* name, type *)
  ...
  "set_debug_callback", {
    default_call with
    args = [ Closure ("debug", debug_closure) ];

So after this change, Closure, Flags, and Enum aren't quite congruent.
---
 generator/generator | 127 ++++++++++++++------------------------------
 1 file changed, 41 insertions(+), 86 deletions(-)

diff --git a/generator/generator b/generator/generator
index 401a451..9dbef2a 100755
--- a/generator/generator
+++ b/generator/generator
@@ -916,6 +916,32 @@ let non_blocking_test_call_description = "\n
 This call does not block, because it returns data that is saved in
 the handle from the NBD protocol handshake."
 
+(* Closures. *)
+let chunk_closure = {
+  cbname = "chunk";
+  cbargs = [ CBBytesIn ("subbuf", "count");
+             CBUInt64 "offset"; CBUInt "status";
+             CBMutable (Int "error") ]
+}
+let completion_closure = {
+  cbname = "completion";
+  cbargs = [ CBMutable (Int "error") ]
+}
+let debug_closure = {
+  cbname = "debug";
+  cbargs = [ CBString "context"; CBString "msg" ]
+}
+let extent_closure = {
+  cbname = "extent";
+  cbargs = [ CBString "metacontext";
+             CBUInt64 "offset";
+             CBArrayAndLen (UInt32 "entries",
+                            "nr_entries");
+             CBMutable (Int "error") ]
+}
+let all_closures = [ chunk_closure; completion_closure;
+                     debug_closure; extent_closure ]
+
 (* Enums. *)
 let tls_enum = {
   enum_prefix = "TLS";
@@ -973,8 +999,7 @@ Return the state of the debug flag on this handle.";
 
   "set_debug_callback", {
     default_call with
-    args = [ Closure { cbname="debug";
-                       cbargs=[CBString "context"; CBString
"msg"] } ];
+    args = [ Closure debug_closure ];
     ret = RErr;
     shortdesc = "set the debug callback";
     longdesc = "\
@@ -1445,10 +1470,7 @@ protocol extensions).";
   "pread_structured", {
     default_call with
     args = [ BytesOut ("buf", "count"); UInt64
"offset";
-             Closure { cbname="chunk";
-                       cbargs=[CBBytesIn ("subbuf",
"count");
-                               CBUInt64 "offset"; CBUInt
"status";
-                               CBMutable (Int "error")] } ];
+             Closure chunk_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RErr;
     permitted_states = [ Connected ];
@@ -1641,13 +1663,7 @@ punching a hole.";
 
   "block_status", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset";
-             Closure { cbname="extent";
-                       cbargs=[CBString "metacontext";
-                               CBUInt64 "offset";
-                               CBArrayAndLen (UInt32 "entries",
-                                              "nr_entries");
-                               CBMutable (Int "error")] } ];
+    args = [ UInt64 "count"; UInt64 "offset"; Closure
extent_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RErr;
     permitted_states = [ Connected ];
@@ -1820,8 +1836,7 @@ C<nbd_pread>.";
   "aio_pread_callback", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Closure { cbname="completion";
-                       cbargs=[CBMutable (Int "error")] } ];
+             Closure completion_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1841,11 +1856,7 @@ completed.  Other parameters behave as documented in
C<nbd_pread>.";
   "aio_pread_structured", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Closure { cbname="chunk";
-                       cbargs=[CBBytesIn ("subbuf",
"count");
-                               CBUInt64 "offset";
-                               CBUInt "status";
-                               CBMutable (Int "error")] } ];
+             Closure chunk_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1862,13 +1873,8 @@ documented in C<nbd_pread_structured>.";
   "aio_pread_structured_callback", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             Closure { cbname="chunk";
-                       cbargs=[CBBytesIn ("subbuf",
"count");
-                               CBUInt64 "offset";
-                               CBUInt "status";
-                               CBMutable (Int "error")] };
-             Closure { cbname="completion";
-                       cbargs=[CBMutable (Int "error")] } ];
+             Closure chunk_closure;
+             Closure completion_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1904,8 +1910,7 @@ C<nbd_pwrite>.";
   "aio_pwrite_callback", {
     default_call with
     args = [ BytesPersistIn ("buf", "count"); UInt64
"offset";
-             Closure { cbname="completion";
-                       cbargs=[CBMutable (Int "error")] } ];
+             Closure completion_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1960,8 +1965,7 @@ Parameters behave as documented in
C<nbd_flush>.";
 
   "aio_flush_callback", {
     default_call with
-    args = [ Closure { cbname="completion";
-                       cbargs=[CBMutable (Int "error")] } ];
+    args = [ Closure completion_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1994,9 +1998,7 @@ Parameters behave as documented in
C<nbd_trim>.";
 
   "aio_trim_callback", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset";
-             Closure { cbname="completion";
-                       cbargs=[CBMutable (Int "error")] } ];
+    args = [ UInt64 "count"; UInt64 "offset"; Closure
completion_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2029,9 +2031,7 @@ Parameters behave as documented in
C<nbd_cache>.";
 
   "aio_cache_callback", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset";
-             Closure { cbname="completion";
-                       cbargs=[CBMutable (Int "error")] } ];
+    args = [ UInt64 "count"; UInt64 "offset"; Closure
completion_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2064,9 +2064,7 @@ Parameters behave as documented in
C<nbd_zero>.";
 
   "aio_zero_callback", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset";
-             Closure { cbname="completion";
-                       cbargs=[CBMutable (Int "error")] } ];
+    args = [ UInt64 "count"; UInt64 "offset"; Closure
completion_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2084,12 +2082,7 @@ Other parameters behave as documented in
C<nbd_zero>.";
 
   "aio_block_status", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset";
-             Closure { cbname="extent";
-                       cbargs=[CBString "metacontext"; CBUInt64
"offset";
-                               CBArrayAndLen (UInt32 "entries",
-                                            "nr_entries");
-                               CBMutable (Int "error")] } ];
+    args = [ UInt64 "count"; UInt64 "offset"; Closure
extent_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2105,13 +2098,7 @@ Parameters behave as documented in
C<nbd_block_status>.";
   "aio_block_status_callback", {
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
-             Closure { cbname="extent";
-                       cbargs=[CBString "metacontext"; CBUInt64
"offset";
-                               CBArrayAndLen (UInt32 "entries",
-                                              "nr_entries");
-                               CBMutable (Int "error")] };
-             Closure { cbname="completion";
-                       cbargs=[CBMutable (Int "error")] } ];
+             Closure extent_closure; Closure completion_closure ];
     optargs = [ OFlags ("flags", cmd_flags) ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -3207,30 +3194,6 @@ let ()         failwithf "%s: optargs can only be
empty list or [OFlags]" name
   ) handle_calls;
 
-  (* Closures must be uniquely named across all calls. *)
-  let () -    let all_args -      List.flatten (List.map (fun (_, { args })
-> args) handle_calls) in
-    let h = Hashtbl.create 13 in
-    List.iter (
-      function
-      | Closure { cbname; cbargs } ->
-         (try
-            (* If we've already added this name to the hash, check
-             * closure args are identical.
-             *)
-            let other_cbargs = Hashtbl.find h cbname in
-            if cbargs <> other_cbargs then
-              failwithf "%s: Closure has different arguments across
methods"
-                cbname
-          with Not_found ->
-                (* Otherwise add it to the hash. *)
-                Hashtbl.add h cbname cbargs
-         )
-      | _ -> ()
-    ) all_args
-  in
-
   (* Check functions using may_set_error. *)
   List.iter (
     function
@@ -3478,20 +3441,12 @@ let print_cbarg_list ?(valid_flag = true) ?(types =
true) cbargs
 (* Callback typedefs in <libnbd.h> *)
 let print_closure_typedefs () -  let all_cls -    List.map (
-      fun (_, { args }) ->
-        filter_map (function Closure cl -> Some cl | _ -> None) args
-    ) handle_calls in
-  let all_cls = List.flatten all_cls in
-  let cmp { cbname = n1 } { cbname = n2 } = compare n1 n2 in
-  let unique_cls = sort_uniq ~cmp all_cls in
   List.iter (
     fun { cbname; cbargs } ->
       pr "typedef int (*nbd_%s_callback) " cbname;
       print_cbarg_list cbargs;
       pr ";\n";
-  ) unique_cls;
+  ) all_closures;
   pr "\n"
 
 let print_extern_and_define name args optargs ret -- 
2.22.0

Richard W.M. Jones

2019-Aug-13 10:06 UTC

head link

[Libguestfs] [PATCH libnbd 2/6] generator: Create only one Python wrapper per closure.

We were previously generating one instance of the Python closure
wrapper per (function * Closure arg).  However these wrappers didn't
actually differ across functions.  We can therefore save a lot of code
by only generating one wrapper per closure globally.

This reduces the amount of generated code by nearly 25%.  Before and
after:

$ wc -l python/methods.c
3275 python/methods.c

$ wc -l python/methods.c
2662 python/methods.c
---
 generator/generator | 237 ++++++++++++++++++++++----------------------
 1 file changed, 117 insertions(+), 120 deletions(-)

diff --git a/generator/generator b/generator/generator
index 9dbef2a..a031bd0 100755
--- a/generator/generator
+++ b/generator/generator
@@ -4108,126 +4108,122 @@ PyInit_libnbdmod (void)
 }
 "
 
+(* Functions with a Closure parameter are special because we
+ * have to generate wrapper functions which translate the
+ * callbacks back to Python.
+ *)
+let print_python_closure_wrapper { cbname; cbargs } +  pr "/* Wrapper for
%s callback. */\n" cbname;
+  pr "static int\n";
+  pr "%s_wrapper " cbname;
+  C.print_cbarg_list cbargs;
+  pr "\n";
+  pr "{\n";
+  pr "  int ret = 0;\n";
+  pr "\n";
+  pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
+  pr "    PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
+  pr "    PyObject *py_args, *py_ret;\n";
+  List.iter (
+    function
+    | CBArrayAndLen (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
+    | CBBytesIn _
+    | CBInt _
+    | CBInt64 _ -> ()
+    | CBMutable (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;
+    | CBString _
+    | CBUInt _
+    | CBUInt64 _ -> ()
+    | CBArrayAndLen _ | CBMutable _ -> assert false
+  ) cbargs;
+  pr "\n";
+
+  pr "    py_args = Py_BuildValue (\"(\"";
+  List.iter (
+    function
+    | CBArrayAndLen (UInt32 n, len) -> pr " \"O\""
+    | CBBytesIn (n, len) -> pr " \"y#\""
+    | CBInt n -> pr " \"i\""
+    | CBInt64 n -> pr " \"L\""
+    | CBMutable (Int n) -> pr " \"O\""
+    | CBString n -> pr " \"s\""
+    | CBUInt n -> pr " \"I\""
+    | CBUInt64 n -> pr " \"K\""
+    | CBArrayAndLen _ | CBMutable _ -> assert false
+  ) cbargs;
+  pr " \")\"";
+  List.iter (
+    function
+    | CBArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
+    | CBBytesIn (n, len) -> pr ", %s, (int) %s" n len
+    | CBMutable (Int n) -> pr ", py_%s" n
+    | CBInt n | CBInt64 n
+    | CBString n
+    | CBUInt n | CBUInt64 n -> pr ", %s" n
+    | CBArrayAndLen _ | CBMutable _ -> 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 ((PyObject *)user_data,
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 "      if (PyLong_Check (py_ret))\n";
+  pr "        ret = PyLong_AsLong (py_ret);\n";
+  pr "      else\n";
+  pr "        /* If it's not a long, just assume it's 0.
*/\n";
+  pr "        ret = 0;\n";
+  pr "      Py_DECREF (py_ret);\n";
+  pr "    }\n";
+  pr "    else {\n";
+  pr "      ret = -1;\n";
+  pr "      PyErr_PrintEx (0); /* print exception */\n";
+  pr "    };\n";
+  pr "\n";
+  List.iter (
+    function
+    | CBArrayAndLen (UInt32 n, _) ->
+       pr "    Py_DECREF (py_%s);\n" n
+    | CBMutable (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
+    | CBBytesIn _
+    | CBInt _ | CBInt64 _
+    | CBString _
+    | CBUInt _ | CBUInt64 _ -> ()
+    | CBArrayAndLen _ | CBMutable _ -> assert false
+  ) cbargs;
+  pr "  }\n";
+  pr "\n";
+  pr "  if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
+  pr "    Py_DECREF ((PyObject *)user_data);\n";
+  pr "\n";
+  pr "  return ret;\n";
+  pr "}\n";
+  pr "\n"
+
+(* Generate the Python binding. *)
 let print_python_binding name { args; optargs; ret; may_set_error } -  (*
Functions with a Closure parameter are special because we
-   * have to generate wrapper functions which translate the
-   * callbacks back to Python.
-   *)
-  List.iter (
-    function
-    | Closure { cbname; cbargs } ->
-       pr "/* Wrapper for %s callback of %s. */\n" cbname name;
-       pr "static int\n";
-       pr "%s_%s_wrapper " name cbname;
-       C.print_cbarg_list cbargs;
-       pr "\n";
-       pr "{\n";
-       pr "  int ret = 0;\n";
-       pr "\n";
-       pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
-       pr "    PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
-       pr "    PyObject *py_args, *py_ret;\n";
-       List.iter (
-         function
-         | CBArrayAndLen (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
-         | CBBytesIn _
-         | CBInt _
-         | CBInt64 _ -> ()
-         | CBMutable (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;
-         | CBString _
-         | CBUInt _
-         | CBUInt64 _ -> ()
-         | CBArrayAndLen _ | CBMutable _ -> assert false
-       ) cbargs;
-       pr "\n";
-
-       pr "    py_args = Py_BuildValue (\"(\"";
-       List.iter (
-         function
-         | CBArrayAndLen (UInt32 n, len) -> pr " \"O\""
-         | CBBytesIn (n, len) -> pr " \"y#\""
-         | CBInt n -> pr " \"i\""
-         | CBInt64 n -> pr " \"L\""
-         | CBMutable (Int n) -> pr " \"O\""
-         | CBString n -> pr " \"s\""
-         | CBUInt n -> pr " \"I\""
-         | CBUInt64 n -> pr " \"K\""
-         | CBArrayAndLen _ | CBMutable _ -> assert false
-       ) cbargs;
-       pr " \")\"";
-       List.iter (
-         function
-         | CBArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
-         | CBBytesIn (n, len) -> pr ", %s, (int) %s" n len
-         | CBMutable (Int n) -> pr ", py_%s" n
-         | CBInt n | CBInt64 n
-         | CBString n
-         | CBUInt n | CBUInt64 n -> pr ", %s" n
-         | CBArrayAndLen _ | CBMutable _ -> 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 ((PyObject *)user_data,
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 "      if (PyLong_Check (py_ret))\n";
-       pr "        ret = PyLong_AsLong (py_ret);\n";
-       pr "      else\n";
-       pr "        /* If it's not a long, just assume it's 0.
*/\n";
-       pr "        ret = 0;\n";
-       pr "      Py_DECREF (py_ret);\n";
-       pr "    }\n";
-       pr "    else {\n";
-       pr "      ret = -1;\n";
-       pr "      PyErr_PrintEx (0); /* print exception */\n";
-       pr "    };\n";
-       pr "\n";
-       List.iter (
-         function
-         | CBArrayAndLen (UInt32 n, _) ->
-            pr "    Py_DECREF (py_%s);\n" n
-         | CBMutable (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
-         | CBBytesIn _
-         | CBInt _ | CBInt64 _
-         | CBString _
-         | CBUInt _ | CBUInt64 _ -> ()
-         | CBArrayAndLen _ | CBMutable _ -> assert false
-       ) cbargs;
-       pr "  }\n";
-       pr "\n";
-       pr "  if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
-       pr "    Py_DECREF ((PyObject *)user_data);\n";
-       pr "\n";
-       pr "  return ret;\n";
-       pr "}\n";
-       pr "\n"
-    | _ -> ()
-  ) args;
-
-  (* Generate the Python binding. *)
   pr "PyObject *\n";
   pr "nbd_internal_py_%s (PyObject *self, PyObject *args)\n" name;
   pr "{\n";
@@ -4390,7 +4386,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }      | BytesPersistIn (n, _)
     | BytesPersistOut (n, _) -> pr ", %s_buf->data,
%s_buf->len" n n
     | Closure { cbname } ->
-       pr ", %s_%s_wrapper" name cbname;
+       pr ", %s_wrapper" cbname;
        pr ", %s_user_data" cbname
     | Enum (n, _) -> pr ", %s" n
     | Flags (n, _) -> pr ", %s_u32" n
@@ -4503,6 +4499,7 @@ let generate_python_methods_c ()    pr "\n";
   pr "#include <methods.h>\n";
   pr "\n";
+  List.iter print_python_closure_wrapper all_closures;
   List.iter (
     fun (name, fn) ->
       print_python_binding name fn
-- 
2.22.0

Richard W.M. Jones

2019-Aug-13 10:06 UTC

head link

[Libguestfs] [PATCH libnbd 3/6] generator: Create only one OCaml wrapper per closure.

Same as previous commit but for OCaml.
---
 generator/generator | 230 ++++++++++++++++++++++----------------------
 1 file changed, 113 insertions(+), 117 deletions(-)

diff --git a/generator/generator b/generator/generator
index a031bd0..7f97163 100755
--- a/generator/generator
+++ b/generator/generator
@@ -5038,123 +5038,118 @@ let print_ocaml_flag_val { flag_prefix; flags }    pr
"}\n";
   pr "\n"
 
+let print_ocaml_closure_wrapper { cbname; cbargs } +  let argnames +   
List.map (
+      function
+      | CBArrayAndLen (UInt32 n, _) | CBBytesIn (n, _)
+      | CBInt n | CBInt64 n
+      | CBMutable (Int n) | CBString n | CBUInt n | CBUInt64 n ->
+         n ^ "v"
+      | CBArrayAndLen _ | CBMutable _ -> assert false
+      ) cbargs in
+
+  pr "/* Wrapper for %s callback. */\n" cbname;
+  pr "static int\n";
+  pr "%s_wrapper_locked " cbname;
+  C.print_cbarg_list ~valid_flag:false cbargs;
+  pr "\n";
+  pr "{\n";
+  pr "  CAMLparam0 ();\n";
+  assert (List.length argnames <= 5);
+  pr "  CAMLlocal%d (%s);\n" (List.length argnames)
+    (String.concat ", " argnames);
+  pr "  CAMLlocal2 (fnv, rv);\n";
+  pr "  int r;\n";
+  pr "  value args[%d];\n" (List.length argnames);
+  pr "\n";
+
+  List.iter (
+    function
+    | CBArrayAndLen (UInt32 n, count) ->
+       pr "  %sv = nbd_internal_ocaml_alloc_int32_array (%s, %s);\n"
+         n n count;
+    | CBBytesIn (n, len) ->
+       pr "  %sv = caml_alloc_string (%s);\n" n len;
+       pr "  memcpy (String_val (%sv), %s, %s);\n" n n len
+    | CBInt n | CBUInt n ->
+       pr "  %sv = Val_int (%s);\n" n n
+    | CBInt64 n ->
+       pr "  %sv = caml_copy_int64 (%s);\n" n n
+    | CBString n ->
+       pr "  %sv = caml_copy_string (%s);\n" n n
+    | CBUInt64 n ->
+       pr "  %sv = caml_copy_int64 (%s);\n" n n
+    | CBMutable (Int n) ->
+       pr "  %sv = caml_alloc_tuple (1);\n" n;
+       pr "  Store_field (%sv, 0, Val_int (*%s));\n" n n
+    | CBArrayAndLen _ | CBMutable _ -> assert false
+  ) cbargs;
+
+  List.iteri (fun i n -> pr "  args[%d] = %s;\n" i n) argnames;
+
+  pr "  fnv = * (value *) user_data;\n";
+
+  pr "  rv = caml_callbackN_exn (fnv, %d, args);\n"
+    (List.length argnames);
+
+  List.iter (
+    function
+    | CBArrayAndLen (UInt32 _, _)
+    | CBBytesIn _
+    | CBInt _
+    | CBInt64 _
+    | CBString _
+    | CBUInt _
+    | CBUInt64 _ -> ()
+    | CBMutable (Int n) ->
+       pr "  *%s = Int_val (Field (%sv, 0));\n" n n
+    | CBArrayAndLen _ | CBMutable _ -> assert false
+  ) cbargs;
+
+  pr "  if (Is_exception_result (rv)) {\n";
+  pr "    /* XXX This is not really an error as callbacks can
return\n";
+  pr "     * an error indication.  But perhaps we should direct
this\n";
+  pr "     * to a more suitable place or formalize what exception\n";
+  pr "     * means error versus unexpected failure.\n";
+  pr "     */\n";
+  pr "    fprintf (stderr,\n";
+  pr "             \"libnbd: uncaught OCaml exception:
%%s\\n\",\n";
+  pr "             caml_format_exception (Extract_exception
(rv)));\n";
+  pr "    CAMLreturnT (int, -1);\n";
+  pr "  }\n";
+
+  pr "\n";
+  pr "  r = Int_val (rv);\n";
+  pr "  assert (r >= 0);\n";
+  pr "  CAMLreturnT (int, r);\n";
+  pr "}\n";
+  pr "\n";
+  pr "static int\n";
+  pr "%s_wrapper " cbname;
+  C.print_cbarg_list cbargs;
+  pr "\n";
+  pr "{\n";
+  pr "  int ret = 0;\n";
+  pr "\n";
+  pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
+  pr "  caml_leave_blocking_section ();\n";
+  pr "  ret = %s_wrapper_locked " cbname;
+  C.print_cbarg_list ~valid_flag:false ~types:false cbargs;
+  pr ";\n";
+  pr "  caml_enter_blocking_section ();\n";
+  pr "  }\n";
+  pr "\n";
+  pr "  if (valid_flag & LIBNBD_CALLBACK_FREE) {\n";
+  pr "    caml_remove_generational_global_root ((value
*)user_data);\n";
+  pr "    free (user_data);\n";
+  pr "  }\n";
+  pr "\n";
+  pr "  return ret;\n";
+  pr "}\n";
+  pr "\n"
+
 let print_ocaml_binding (name, { args; optargs; ret }) -  (* Functions with a
callback parameter require special handling. *)
-  List.iter (
-    function
-    | Closure { cbname; cbargs } ->
-       let argnames -         List.map (
-           function
-           | CBArrayAndLen (UInt32 n, _) | CBBytesIn (n, _)
-           | CBInt n | CBInt64 n
-           | CBMutable (Int n) | CBString n | CBUInt n | CBUInt64 n ->
-              n ^ "v"
-           | CBArrayAndLen _ | CBMutable _ -> assert false
-         ) cbargs in
-
-       pr "/* Wrapper for %s callback of %s. */\n" cbname name;
-       pr "static int\n";
-       pr "%s_%s_wrapper_locked " name cbname;
-       C.print_cbarg_list ~valid_flag:false cbargs;
-       pr "\n";
-       pr "{\n";
-       pr "  CAMLparam0 ();\n";
-       assert (List.length argnames <= 5);
-       pr "  CAMLlocal%d (%s);\n" (List.length argnames)
-          (String.concat ", " argnames);
-       pr "  CAMLlocal2 (fnv, rv);\n";
-       pr "  int r;\n";
-       pr "  value args[%d];\n" (List.length argnames);
-       pr "\n";
-
-       List.iter (
-         function
-         | CBArrayAndLen (UInt32 n, count) ->
-            pr "  %sv = nbd_internal_ocaml_alloc_int32_array (%s,
%s);\n"
-               n n count;
-         | CBBytesIn (n, len) ->
-            pr "  %sv = caml_alloc_string (%s);\n" n len;
-            pr "  memcpy (String_val (%sv), %s, %s);\n" n n len
-         | CBInt n | CBUInt n ->
-            pr "  %sv = Val_int (%s);\n" n n
-         | CBInt64 n ->
-            pr "  %sv = caml_copy_int64 (%s);\n" n n
-         | CBString n ->
-            pr "  %sv = caml_copy_string (%s);\n" n n
-         | CBUInt64 n ->
-            pr "  %sv = caml_copy_int64 (%s);\n" n n
-         | CBMutable (Int n) ->
-            pr "  %sv = caml_alloc_tuple (1);\n" n;
-            pr "  Store_field (%sv, 0, Val_int (*%s));\n" n n
-         | CBArrayAndLen _ | CBMutable _ -> assert false
-       ) cbargs;
-
-       List.iteri (fun i n -> pr "  args[%d] = %s;\n" i n)
argnames;
-
-       pr "  fnv = * (value *) user_data;\n";
-
-       pr "  rv = caml_callbackN_exn (fnv, %d, args);\n"
-          (List.length argnames);
-
-       List.iter (
-         function
-         | CBArrayAndLen (UInt32 _, _)
-         | CBBytesIn _
-         | CBInt _
-         | CBInt64 _
-         | CBString _
-         | CBUInt _
-         | CBUInt64 _ -> ()
-         | CBMutable (Int n) ->
-            pr "  *%s = Int_val (Field (%sv, 0));\n" n n
-         | CBArrayAndLen _ | CBMutable _ -> assert false
-       ) cbargs;
-
-       pr "  if (Is_exception_result (rv)) {\n";
-       pr "    /* XXX This is not really an error as callbacks can
return\n";
-       pr "     * an error indication.  But perhaps we should direct
this\n";
-       pr "     * to a more suitable place or formalize what
exception\n";
-       pr "     * means error versus unexpected failure.\n";
-       pr "     */\n";
-       pr "    fprintf (stderr,\n";
-       pr "             \"libnbd: uncaught OCaml exception:
%%s\\n\",\n";
-       pr "             caml_format_exception (Extract_exception
(rv)));\n";
-       pr "    CAMLreturnT (int, -1);\n";
-       pr "  }\n";
-
-       pr "\n";
-       pr "  r = Int_val (rv);\n";
-       pr "  assert (r >= 0);\n";
-       pr "  CAMLreturnT (int, r);\n";
-       pr "}\n";
-       pr "\n";
-       pr "static int\n";
-       pr "%s_%s_wrapper " name cbname;
-       C.print_cbarg_list cbargs;
-       pr "\n";
-       pr "{\n";
-       pr "  int ret = 0;\n";
-       pr "\n";
-       pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
-       pr "  caml_leave_blocking_section ();\n";
-       pr "  ret = %s_%s_wrapper_locked " name cbname;
-       C.print_cbarg_list ~valid_flag:false ~types:false cbargs;
-       pr ";\n";
-       pr "  caml_enter_blocking_section ();\n";
-       pr "  }\n";
-       pr "\n";
-       pr "  if (valid_flag & LIBNBD_CALLBACK_FREE) {\n";
-       pr "    caml_remove_generational_global_root ((value
*)user_data);\n";
-       pr "    free (user_data);\n";
-       pr "  }\n";
-       pr "\n";
-       pr "  return ret;\n";
-       pr "}\n";
-       pr "\n"
-    | _ -> ()
-  ) args;
-
   (* Get the names of all the value arguments including the handle. *)
   let values      List.map ocaml_name_of_optarg optargs @ ["h"] @
@@ -5233,7 +5228,7 @@ let print_ocaml_binding (name, { args; optargs; ret })    
pr "  if (%s_user_data == NULL) caml_raise_out_of_memory ();\n"
cbname;
        pr "  *%s_user_data = %sv;\n" cbname cbname;
        pr "  caml_register_generational_global_root
(%s_user_data);\n" cbname;
-       pr "  const void *%s_callback = %s_%s_wrapper;\n" cbname name
cbname
+       pr "  const void *%s_callback = %s_wrapper;\n" cbname cbname
     | Enum (n, { enum_prefix }) ->
        pr "  int %s = %s_val (%sv);\n" n enum_prefix n
     | Flags (n, { flag_prefix }) ->
@@ -5352,6 +5347,7 @@ let generate_ocaml_nbd_c ()    pr "#pragma GCC
diagnostic ignored \"-Wmissing-prototypes\"\n";
   pr "\n";
 
+  List.iter print_ocaml_closure_wrapper all_closures;
   List.iter print_ocaml_enum_val all_enums;
   List.iter print_ocaml_flag_val all_flags;
   List.iter print_ocaml_binding handle_calls
-- 
2.22.0

Richard W.M. Jones

2019-Aug-13 10:06 UTC

head link

[Libguestfs] [PATCH libnbd 4/6] lib: Check Closure parameter is not NULL.

This was not permitted by the API before, but would in some
circumstances work.
---
 generator/generator | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/generator/generator b/generator/generator
index 7f97163..01da1c3 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3664,6 +3664,16 @@ let generate_lib_api_c ()      in
     List.iter (
       function
+      | Closure { cbname } ->
+         let value = match errcode with
+           | Some value -> value
+           | None -> assert false in
+         pr "  if (%s_callback == NULL) {\n" cbname;
+         pr "    set_error (EFAULT, \"%%s cannot be NULL\",
\"%s\");\n" cbname;
+         pr "    ret = %s;\n" value;
+         pr "    goto out;\n";
+         pr "  }\n";
+         need_out_label := true
       | Enum (n, { enum_prefix; enums }) ->
          let value = match errcode with
            | Some value -> value
-- 
2.22.0

Richard W.M. Jones

2019-Aug-13 10:06 UTC

head link

[Libguestfs] [PATCH libnbd 5/6] generator: Implement OClosure.

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

diff --git a/generator/generator b/generator/generator
index 01da1c3..3add9a4 100755
--- a/generator/generator
+++ b/generator/generator
@@ -867,6 +867,7 @@ and arg  | UInt32 of string         (* 32 bit unsigned int
*)
 | UInt64 of string         (* 64 bit unsigned int *)
 and optarg +| OClosure of closure      (* optional closure *)
 | OFlags of string * flags (* optional flags, uint32_t in C *)
 and ret  | RBool                    (* return a boolean, or error *)
@@ -3184,16 +3185,6 @@ end = struct
 
 (* Check the API definition. *)
 let () -  (* Currently optargs can only be [] or [OFlags].  This condition
-   * will be relaxed later when we support more optional arguments.
-   *)
-  List.iter (
-    function
-    | _, { optargs = [] } | _, { optargs = [OFlags _] } -> ()
-    | (name, _) ->
-       failwithf "%s: optargs can only be empty list or [OFlags]"
name
-  ) handle_calls;
-
   (* Check functions using may_set_error. *)
   List.iter (
     function
@@ -3377,6 +3368,12 @@ let rec print_arg_list ?(handle = false) ?(types = true)
args optargs        if !comma then pr ", ";
       comma := true;
       match optarg with
+      | OClosure { cbname; cbargs } ->
+         if types then pr "nbd_%s_callback " cbname;
+         pr "%s_callback" cbname;
+         pr ", ";
+         if types then pr "void *";
+         pr "%s_user_data" cbname
       | OFlags (n, _) ->
          if types then pr "uint32_t ";
          pr "%s" n
@@ -3707,6 +3704,7 @@ let generate_lib_api_c ()      ) args;
     List.iter (
       function
+      | OClosure _ -> ()
       | OFlags (n, flags) ->
          print_flags_check n flags
     ) optargs;
@@ -3756,6 +3754,7 @@ let generate_lib_api_c ()      ) args;
     List.iter (
       function
+      | OClosure { cbname } -> pr " %s=%%s" cbname
       | OFlags (n, _) -> pr " %s=0x%%x" n
     ) optargs;
     pr "\"";
@@ -3778,6 +3777,7 @@ let generate_lib_api_c ()      ) args;
     List.iter (
       function
+      | OClosure { cbname } -> pr ", %s_callback ?
\"<fun>\" : \"NULL\"" cbname
       | OFlags (n, _) -> pr ", %s" n
     ) optargs;
     pr ");\n"
@@ -4286,6 +4286,8 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
+    | OClosure { cbname } ->
+       pr "  PyObject *%s_user_data;\n" cbname
     | OFlags (n, _) ->
        pr "  uint32_t %s_u32;\n" n;
        pr "  unsigned int %s; /* really uint32_t */\n" n
@@ -4316,6 +4318,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
+    | OClosure _ -> pr " \"O\""
     | OFlags _ -> pr " \"I\""
   ) optargs;
   pr "\n";
@@ -4342,6 +4345,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
+    | OClosure { cbname } -> pr ", &%s_user_data" cbname
     | OFlags (n, _) -> pr ", &%s" n
   ) optargs;
   pr "))\n";
@@ -4383,6 +4387,16 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
+    | OClosure { cbname } ->
+       pr "  if (%s_user_data) {\n" cbname;
+       pr "    /* Increment refcount since pointer may be saved by libnbd.
*/\n";
+       pr "    Py_INCREF (%s_user_data);\n" cbname;
+       pr "    if (!PyCallable_Check (%s_user_data)) {\n" cbname;
+       pr "      PyErr_SetString (PyExc_TypeError,\n";
+       pr "                       \"callback parameter %s is not
callable\");\n" cbname;
+       pr "      return NULL;\n";
+       pr "    }\n";
+       pr "  }\n"
     | OFlags (n, _) -> pr "  %s_u32 = %s;\n" n n
   ) optargs;
 
@@ -4412,6 +4426,9 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
+    | OClosure { cbname } ->
+       pr ", %s_user_data ? %s_wrapper : NULL" cbname cbname;
+       pr ", %s_user_data" cbname
     | OFlags (n, _) -> pr ", %s_u32" n
   ) optargs;
   pr ");\n";
@@ -4668,6 +4685,7 @@ class NBD (object):
       let optargs          List.map (
           function
+          | OClosure { cbname } -> cbname, Some "None", None
           | OFlags (n, _) -> n, Some "0", None
         ) optargs in
       let args = args @ optargs in
@@ -4756,6 +4774,8 @@ and ocaml_ret_to_string = function
   | RUInt -> "int"
 
 and ocaml_optarg_to_string = function
+  | OClosure { cbname; cbargs } ->
+     sprintf "?%s:(%s)" cbname (ocaml_closuredecl_to_string cbargs)
   | OFlags (n, { flag_prefix }) -> sprintf "?%s:%s.t list" n
flag_prefix
 
 and ocaml_closuredecl_to_string cbargs @@ -4794,6 +4814,7 @@ let
ocaml_name_of_arg = function
   | UInt64 n -> n
 
 let ocaml_name_of_optarg = function
+  | OClosure { cbname } -> cbname
   | OFlags (n, _) -> n
 
 let num_params args optargs @@ -5202,6 +5223,19 @@ let print_ocaml_binding
(name, { args; optargs; ret })
   List.iter (
     function
+    | OClosure { cbname } ->
+       pr "  const void *%s_callback = NULL;\n" cbname;
+       pr "  value *%s_user_data = NULL;\n" cbname;
+       pr "  if (%sv != Val_int (0)) { /* Some closure */\n" cbname;
+       pr "    /* The function may save a reference to the closure, so
we\n";
+       pr "     * must treat it as a possible GC root.\n";
+       pr "     */\n";
+       pr "    %s_user_data = malloc (sizeof (value));\n" cbname;
+       pr "    if (%s_user_data == NULL) caml_raise_out_of_memory
();\n" cbname;
+       pr "    *%s_user_data = Field (%sv, 0);\n" cbname cbname;
+       pr "    caml_register_generational_global_root
(%s_user_data);\n" cbname;
+       pr "    %s_callback = %s_wrapper;\n" cbname cbname;
+       pr "  }\n";
     | OFlags (n, { flag_prefix }) ->
        pr "  uint32_t %s;\n" n;
        pr "  if (%sv != Val_int (0)) /* Some [ list of %s.t ] */\n"
-- 
2.22.0

Richard W.M. Jones

2019-Aug-13 10:06 UTC

head link

[Libguestfs] [PATCH libnbd 6/6] lib: Make all completion callbacks into OClosures.

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

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

Eric Blake

2019-Aug-13 11:12 UTC

head link

Re: [Libguestfs] [PATCH libnbd 1/6] generator: Share single list of all Closures.

On 8/13/19 5:06 AM, Richard W.M. Jones wrote:> This change does not affect the output of the generator.
> 
> Note this requires that all Closure args have the same parameter name
> whichever method they appear in.  An alternate refactoring could work
> the same way as Flags and Enum where the parameter name is part of the
> arg, eg:
> 
>   type arg >   ...
>   | Closure of string * closure   (* name, type *)
>   ...
>   "set_debug_callback", {
>     default_call with
>     args = [ Closure ("debug", debug_closure) ];
> 
> So after this change, Closure, Flags, and Enum aren't quite congruent.
But the difference isn't too bad.
> ---
>  generator/generator | 127 ++++++++++++++------------------------------
>  1 file changed, 41 insertions(+), 86 deletions(-)
The diffstat shows this is a win.

ACK

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

Eric Blake

2019-Aug-13 11:15 UTC

head link

Re: [Libguestfs] [PATCH libnbd 2/6] generator: Create only one Python wrapper per closure.

On 8/13/19 5:06 AM, Richard W.M. Jones wrote:> We were previously generating one instance of the Python closure
> wrapper per (function * Closure arg).  However these wrappers didn't
> actually differ across functions.  We can therefore save a lot of code
> by only generating one wrapper per closure globally.
> 
> This reduces the amount of generated code by nearly 25%.  Before and
> after:
> 
> $ wc -l python/methods.c
> 3275 python/methods.c
> 
> $ wc -l python/methods.c
> 2662 python/methods.c
Keeping it as a separate patch makes sense.  It's a definite win.

ACK.

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

Eric Blake

2019-Aug-13 11:26 UTC

head link

Re: [Libguestfs] [PATCH libnbd 4/6] lib: Check Closure parameter is not NULL.

On 8/13/19 5:06 AM, Richard W.M. Jones wrote:> This was not permitted by the API before, but would in some
> circumstances work.
> ---
>  generator/generator | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
Requires nbd_pread_structured and nbd_block_status to pass in a callback
(good).  Requires nbd_set_debug_callback to provide a non-NULL pointer
(a bit rough, as now you can't pass NULL to deinstall your handler and
get back to the default behavior - but how often are applications likely
to deinstall a debug handler? If it becomes a problem, we can improve
the 'closure' type to take a bool argument stating whether the closure
accepts NULL, defaulting to false, but set to true for debug_closure -
or we could rewrite nbd_set_debug_callback to take an OClosure instead
of a Closure).  Requires completion_callback to be non-NULL, but we
relax that restriction by moving to OClosure.

ACK.

Yes, the first four can be applied no matter what else we discuss.
> diff --git a/generator/generator b/generator/generator
> index 7f97163..01da1c3 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -3664,6 +3664,16 @@ let generate_lib_api_c () >      in
>      List.iter (
>        function
> +      | Closure { cbname } ->
> +         let value = match errcode with
> +           | Some value -> value
> +           | None -> assert false in
The generator has a check that:

    (* !may_set_error is incompatible with String parameters because
     * we have to do a NULL-check on those which may return an error.
     *)

Should this check be widened to cover Closure, so that we get a nicer
error than 'assert false' if someone goofs in adding a new signature
that uses Closure with the wrong return type?  Can be a separate patch,
since we missed doing it for Enum and Flags in earlier patches, which
also are instances where we used 'assert false' and require the ability
to return an error.  Doesn't affect runtime correctness, just
ease-of-use when re-compiling changes to the generator.

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

Eric Blake

2019-Aug-13 11:34 UTC

head link

Re: [Libguestfs] [PATCH libnbd 5/6] generator: Implement OClosure.

On 8/13/19 5:06 AM, Richard W.M. Jones wrote:> An optional Closure parameter, but otherwise works the same way as
> Closure.
> @@ -3778,6 +3777,7 @@ let generate_lib_api_c () >      ) args;
>      List.iter (
>        function
> +      | OClosure { cbname } -> pr ", %s_callback ?
\"<fun>\" : \"NULL\"" cbname
Well, it also permits a NULL fn pointer.
> @@ -4383,6 +4387,16 @@ let print_python_binding name { args; optargs; ret;
may_set_error } >    ) args;
>    List.iter (
>      function
> +    | OClosure { cbname } ->
> +       pr "  if (%s_user_data) {\n" cbname;
> +       pr "    /* Increment refcount since pointer may be saved by
libnbd. */\n";
> +       pr "    Py_INCREF (%s_user_data);\n" cbname;
> +       pr "    if (!PyCallable_Check (%s_user_data)) {\n"
cbname;
I don't think PyNone is callable; this probably needs to gain a special
case for when the user omitted the optional argument and we thus...
> +       pr "      PyErr_SetString (PyExc_TypeError,\n";
> +       pr "                       \"callback parameter %s is not
callable\");\n" cbname;
> +       pr "      return NULL;\n";
> +       pr "    }\n";
> +       pr "  }\n"
>      | OFlags (n, _) -> pr "  %s_u32 = %s;\n" n n
>    ) optargs;
>  
> @@ -4412,6 +4426,9 @@ let print_python_binding name { args; optargs; ret;
may_set_error } >    ) args;
>    List.iter (
>      function
> +    | OClosure { cbname } ->
> +       pr ", %s_user_data ? %s_wrapper : NULL" cbname cbname;
> +       pr ", %s_user_data" cbname
>      | OFlags (n, _) -> pr ", %s_u32" n
>    ) optargs;
>    pr ");\n";
> @@ -4668,6 +4685,7 @@ class NBD (object):
>        let optargs >          List.map (
>            function
> +          | OClosure { cbname } -> cbname, Some "None", None
...passed in the default Python 'None' in its place.

> @@ -5202,6 +5223,19 @@ let print_ocaml_binding (name, { args; optargs; ret
}) >
>    List.iter (
>      function
> +    | OClosure { cbname } ->
> +       pr "  const void *%s_callback = NULL;\n" cbname;
> +       pr "  value *%s_user_data = NULL;\n" cbname;
> +       pr "  if (%sv != Val_int (0)) { /* Some closure */\n"
cbname;
But for OCaml you got it right - you are handling 'None' vs. 'Some
closure'.

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

Eric Blake

2019-Aug-13 11:41 UTC

head link

Re: [Libguestfs] [PATCH libnbd 6/6] lib: Make all completion callbacks into OClosures.

On 8/13/19 5:06 AM, Richard W.M. Jones wrote:> This doesn't change the C API, except that it is now permitted to pass
> NULL, NULL for the completion callback.  For the Python and OCaml
> APIs the parameter changes to be an optional parameter.
As mentioned elsewhere in this series, we probably want to move
nbd_set_debug_callback to take an OClosure as well, to preserve
pre-series behavior of allowing a NULL fn to revert back to the default
debug handler.

> @@ -1874,9 +1873,8 @@ documented in C<nbd_pread_structured>.";
>    "aio_pread_structured_callback", {
>      default_call with
>      args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
> -             Closure chunk_closure;
> -             Closure completion_closure ];
> -    optargs = [ OFlags ("flags", cmd_flags) ];
> +             Closure chunk_closure ];
> +    optargs = [ OClosure completion_closure; OFlags ("flags",
cmd_flags) ];
If we are too bothered by the inconsistency mentioned in patch 1 (in
that all closure names are fixed globally), we would rewrite this to

optargs = [ OClosure ("completion", completion_closure);
            OFlags ("flags", cmd_flags) ];

but I'm fine with what you have.

ACK.

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

Eric Blake

2019-Aug-13 11:50 UTC

head link

Re: [Libguestfs] [PATCH 0/6] Implement OClosure.

On 8/13/19 5:06 AM, Richard W.M. Jones wrote:> Patches 1-4 are basically uncontroversial, straightforward refactoring
> and IMHO we should just push them.  Possibly 1-3 should be squashed
> together, but I posted them separately so they are easier to review.
Agreed.
> 
> Patches 5 and 6 together implement OClosure.  Patch 5 adds the feature
> and is simple to understand.
> 
> Patch 6 changes the Closure completion callbacks into OClosure, but
> because it doesn't change the position within the list of arguments
> the C API doesn't change at all.  The Python API also doesn't
change
> for the same reason.  (The OCaml API _does_ change but not
> significantly).
> 
> I've run out of time on this patch today because I've got meetings
for
> the rest of the day (and it's only 11am!)  However if I did have more
> time there would be a 7th patch in the series which renames all
> aio_*_callback functions to simply aio_*.  eg. nbd_aio_pread_callback
> is renamed to nbd_aio_pread (and old nbd_aio_pread is removed).  That
> is obviously a somewhat large albeit it mechanical API change.
Then from there, we move on to a patch that changes Closure/OClosure to
switch the C bindings from 2-tuple to 3-tuple, where you provide a
separate free_fn to be called at the end-of-lifetime instead of the
current valid_flags parameter to the single function.

I'm liking the way this is headed, even though it is a big API/ABI
change to async users.

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

Seemingly Similar Threads

Search for more apparently analagous threads

Libguestfs - Aug 2019 - [PATCH libnbd 1/6] generator: Share single list of all Closures.

[Libguestfs] [PATCH 0/6] Implement OClosure.

[Libguestfs] [PATCH libnbd 1/6] generator: Share single list of all Closures.

[Libguestfs] [PATCH libnbd 2/6] generator: Create only one Python wrapper per closure.

[Libguestfs] [PATCH libnbd 3/6] generator: Create only one OCaml wrapper per closure.

[Libguestfs] [PATCH libnbd 4/6] lib: Check Closure parameter is not NULL.

[Libguestfs] [PATCH libnbd 5/6] generator: Implement OClosure.

[Libguestfs] [PATCH libnbd 6/6] lib: Make all completion callbacks into OClosures.

Re: [Libguestfs] [PATCH libnbd 1/6] generator: Share single list of all Closures.

Re: [Libguestfs] [PATCH libnbd 2/6] generator: Create only one Python wrapper per closure.

Re: [Libguestfs] [PATCH libnbd 4/6] lib: Check Closure parameter is not NULL.

Re: [Libguestfs] [PATCH libnbd 5/6] generator: Implement OClosure.

Re: [Libguestfs] [PATCH libnbd 6/6] lib: Make all completion callbacks into OClosures.

Re: [Libguestfs] [PATCH 0/6] Implement OClosure.

Seemingly Similar Threads