10:02 a.m.
On 7/24/19 7:17 AM, Richard W.M. Jones wrote:
In preparation for closure lifetimes, split up the Closure so it no
longer describes a list of closures, but a single callback.
This changes the API because functions which take 2 or more closures
now pass a separate user_data for each one.
---
docs/libnbd.pod | 3 +-
examples/strict-structured-reads.c | 2 +-
generator/generator | 760 ++++++++++++----------------
generator/states-reply-simple.c | 2 +-
generator/states-reply-structured.c | 8 +-
lib/internal.h | 3 +-
lib/rw.c | 32 +-
7 files changed, 364 insertions(+), 446 deletions(-)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index 5608e63..631bb3b 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -487,8 +487,7 @@ C<nbd_set_debug_callback>, C<nbd_pread_callback>).
Libnbd can call
these functions while processing.
Callbacks have an opaque C<void *user_data> pointer. This is passed
Should we tweak this to explicitly call out "Callbacks in the C
language", to differentiate it from bindings to other languages that do
not do this?
-as the first parameter to the callback. libnbd functions that take
-two callback pointers share the same opaque data for both calls.
+as the first parameter to the callback.
+++ b/generator/generator
@@ -849,10 +849,10 @@ and arg =
written by the function *)
| BytesPersistIn of string * string (* same as above, but buffer persists *)
| BytesPersistOut of string * string
-| Closure of bool * closure list (* void *opaque + one or more closures
- flag if true means callbacks persist
- in the handle, false means they only
- exist during the function call *)
+| Closure of bool * closure (* function pointer + void *opaque
+ flag if true means callbacks persist
+ in the handle, false means they only
+ exist during the function call *)
Do we still need the closure record type, or can/should we simplify
further to:
| Closure of bool * string * arg list
(and later in the series, even further by dropping 'bool *')
| Flags of string (* NBD_CMD_FLAG_* flags *)
| Int of string (* small int *)
| Int64 of string (* 64 bit signed int *)
@@ -921,8 +921,8 @@ Return the state of the debug flag on this handle.";
"set_debug_callback", {
default_call with
args = [ Closure (true,
- [{ cbname="debug_fn";
- cbargs=[String "context"; String "msg"] }])
];
+ { cbname="debug_fn";
+ cbargs=[String "context"; String "msg"] })
];
Keeping the record type means that you still have to provide names, vs.
directly using tuples where we'd write:
args = [ Closure (true, "debug_fn",
[String "context"; String "msg"]) ];
@@ -3811,154 +3795,140 @@ let print_python_binding name { args; ret }
=
*)
List.iter (
function
- | Closure (persistent, cls) ->
- pr "struct %s_user_data {\n" name;
- List.iter (
- fun { cbname } ->
- pr " PyObject *%s;\n" cbname
- ) cls;
- pr "};\n";
- pr "\n";
-
+ | Closure (persistent, { cbname; cbargs }) ->
(* Persistent closures need an explicit function to decrement
* the closure refcounts and free the user_data struct.
Comment needs a touchup now that there is no user_data struct.
*)
if persistent then (
pr "static void\n";
- pr "free_%s_user_data (void *vp)\n" name;
+ pr "free_%s_%s_user_data (void *vp)\n" name cbname;
pr "{\n";
- pr " struct %s_user_data *user_data = vp;\n" name;
+ pr " PyObject *user_data = vp;\n";
pr "\n";
- List.iter (
- fun { cbname } ->
- pr " Py_DECREF (user_data->%s);\n" cbname
- ) cls;
- pr " free (user_data);\n";
+ pr " Py_DECREF (user_data);\n";
pr "}\n";
pr "\n";
);
... (lots of churn due to reindentation, such is life)
+ pr " py_args = Py_BuildValue (\"(\"";
+ List.iter (
+ function
+ | ArrayAndLen (UInt32 n, len) -> pr " \"O\""
+ | BytesIn (n, len) -> pr " \"y#\""
+ | Int n -> pr " \"i\""
+ | Int64 n -> pr " \"L\""
+ | Mutable (Int n) -> pr " \"O\""
+ | String n -> pr " \"s\""
+ | UInt64 n -> pr " \"K\""
+ (* The following not yet implemented for callbacks XXX *)
+ | ArrayAndLen _ | Bool _ | BytesOut _
+ | BytesPersistIn _ | BytesPersistOut _
Not your usual indentation style.
+ | Closure _
+ | Flags _ | Mutable _
+ | Path _ | SockAddrAndLen _ | StringList _
+ | UInt _ | UInt32 _ -> assert false
+ ) cbargs;
+ pr " \")\"";
+ List.iter (
+ function
+ | ArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
+ | BytesIn (n, len) -> pr ", %s, (int) %s" n len
+ | Mutable (Int n) -> pr ", py_%s" n
+ | Int n | Int64 n
+ | String n
+ | UInt64 n -> pr ", %s" n
+ (* The following not yet implemented for callbacks XXX *)
+ | ArrayAndLen _ | Bool _ | BytesOut _
+ | BytesPersistIn _ | BytesPersistOut _
+ | Closure _
+ | Flags _ | Mutable _
+ | Path _ | SockAddrAndLen _ | StringList _
+ | UInt _ | UInt32 _ -> assert false
More instances of what appears to be emacs indenting differently from
your preferences.
@@ -4031,19 +3998,6 @@ let print_python_binding name { args; ret } =
) args;
pr "\n";
- (* Allocate the persistent closure user_data. *)
- List.iter (
- function
- | Closure (false, _) -> ()
- | Closure (true, cls) ->
- pr " user_data = malloc (sizeof *user_data);\n";
- pr " if (user_data == NULL) {\n";
- pr " PyErr_NoMemory ();\n";
- pr " return NULL;\n";
- pr " }\n"
- | _ -> ()
- ) args;
Nice that we get rid of this.
Overall, the patch makes sense; I assume you can fix any problems I
pointed out above without needing to send v2 (especially since this is
mostly OCaml, where I defer to your experience anyway)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org