[Libguestfs] [PATCH libnbd 1/3] generator: Change Closure so it describes single callbacks.
Eric Blake
eblake at redhat.com
Wed Jul 24 15:02:04 UTC 2019
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190724/59440856/attachment.sig>
More information about the Libguestfs
mailing list