[Libguestfs] [PATCH libnbd v2] generator: Define new Closure type instead of callbacks.
Eric Blake
eblake at redhat.com
Tue Jul 16 14:03:55 UTC 2019
On 7/16/19 6:04 AM, Richard W.M. Jones wrote:
> A Closure is a list of (usually one, but can be more) closures. In C
> there is also a singe ‘void *user_data’ parameter which is passed by
> the caller into the function and through as the first parameter of
> each callback invocation.
>
> By grouping the previously separate Opaque and Callback* parameters
> together we can avoid the awkward situation where we have to scan
> through the argument list to try to match them up.
>
> Because this is a closure, in non-C languages we can simply map these
> to a closure and drop the opaque parameter entirely. It is not needed
> since languages with proper closures can capture local state in the
> closure.
>
> Unlike the previous code it is no longer possible to mix persistent
> and non-persistent callbacks in the same API call. This was not used
> before and is unlikely to be useful.
>
> For the C API there is no API or ABI change (the only change is the
> naming of the opaque pointer which is not part of the API). For the
> non-C languages the opaque parameter is no longer required as
> discussed above.
>
> Partly based on Eric Blake's earlier work here:
> https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00160
> ---
> @@ -863,6 +864,10 @@ and arg =
> | UInt of string (* small unsigned int *)
> | UInt32 of string (* 32 bit unsigned int *)
> | UInt64 of string (* 64 bit unsigned int *)
> +and closure = {
> + cbname : string; (* name of callback function *)
> + cbargs : arg list; (* all closures return int for now *)
Of course, now we could decide to make nbd_add_close_callback take a
closure that returns void, since we ignore the return value there. But
that can be done on top, if at all.
> +}
> and ret =
> | RBool (* return a boolean, or error *)
> | RConstString (* return a const string, NULL for error *)
> @@ -915,9 +920,9 @@ Return the state of the debug flag on this handle.";
>
> "set_debug_callback", {
> default_call with
> - args = [ Opaque "data";
> - CallbackPersist ("debug_fn", [Opaque "data";
> - String "context"; String "msg"]) ];
> + args = [ Closure (true,
> + [{ cbname="debug_fn";
> + cbargs=[String "context"; String "msg"] }]) ];
One style of spacing (the list starts with '[{ ', and cbargs has no space),
> ret = RErr;
> shortdesc = "set the debug callback";
> longdesc = "\
> @@ -1345,10 +1350,11 @@ protocol extensions).";
> "pread_structured", {
> default_call with
> args = [ BytesOut ("buf", "count"); UInt64 "offset";
> - Opaque "data";
> - Callback ("chunk", [ Opaque "data"; BytesIn ("subbuf", "count");
> + Closure (false,
> + [{ cbname="chunk";
> + cbargs=[ BytesIn ("subbuf", "count");
a second here (list start is same, cbargs now has a space after [)
> UInt64 "offset"; Int "status";
> - Mutable (Int "error"); ]);
> + Mutable (Int "error")] }]);
> Flags "flags" ];
> ret = RErr;
> permitted_states = [ Connected ];
> @@ -1534,12 +1540,13 @@ punching a hole.";
> "block_status", {
> default_call with
> args = [ UInt64 "count"; UInt64 "offset";
> - Opaque "data";
> - Callback ("extent", [Opaque "data"; String "metacontext";
> - UInt64 "offset";
> - ArrayAndLen (UInt32 "entries",
> - "nr_entries");
> - Mutable (Int "error")]);
> + Closure (false,
> + [ {cbname="extent";
> + cbargs=[String "metacontext";
and a third here (list starts with '[ {', and cbargs has no space).
Probably worth picking a style you like and then using it consistently,
but that's cosmetic and doesn't affect the patch's operation.
> @@ -3143,19 +3151,19 @@ let generate_lib_libnbd_syms () =
> pr " local: *;\n";
> pr "};\n"
>
> -let rec name_of_arg = function
> -| ArrayAndLen (arg, n) -> name_of_arg arg @ [n]
> +let rec c_name_of_arg = function
> +| ArrayAndLen (arg, n) -> c_name_of_arg arg @ [n]
> | Bool n -> [n]
> | BytesIn (n, len) -> [n; len]
> | BytesOut (n, len) -> [n; len]
> | BytesPersistIn (n, len) -> [n; len]
> | BytesPersistOut (n, len) -> [n; len]
> -| Callback (n, _) | CallbackPersist (n, _) -> [n]
> +| Closure (_, closures) ->
> + "user_data" :: List.map (fun { cbname } -> cbname) closures
This hard-coded parameter name (in C) implies that you can have at most
one Closure in an arg list. Should we enforce that (the same way we
enforce that there is at most one Flags)?
> @@ -3164,13 +3172,18 @@ let rec name_of_arg = function
> | UInt32 n -> [n]
> | UInt64 n -> [n]
>
> -let rec print_c_arg_list ?(handle = false) args =
> +let rec print_c_arg_list ?(handle = false) ?(user_data = false) args =
> pr "(";
> let comma = ref false in
> if handle then (
> comma := true;
> pr "struct nbd_handle *h";
> );
> + if user_data then (
> + if !comma then pr ", ";
> + comma := true;
> + pr "void *user_data";
> + );
A bit different from my approach, but it works. Less change to existing
callers, at any rate.
> @@ -3560,9 +3577,9 @@ are called is not defined. This API is only available
> from C and is designed to help when writing bindings to libnbd
> from other programming languages.
>
> - typedef void (*nbd_close_callback) (void *data);
> + typedef void (*nbd_close_callback) (void *user_data);
I might have split out the rename churn (s/\(data\|opaque\)/user_data/)
in a separate patch; but it's probably not worth it now.
> let print_python_binding name { args; ret } =
> - (* Functions with a callback parameter are special because we
> - * have to generate a wrapper function which translates the
> - * callback parameters back to Python.
> + (* Functions with a Closure parameter are special because we
> + * have to generate wrapper functions which translate the
> + * callbacks back to Python.
> *)
> - pr "\n";
> + (* Persistent closures need an explicit function to decrement
> + * the closure refcounts and free the user_data struct.
> + *)
> + if persistent then (
> + pr "static void\n";
> + pr "free_%s_user_data (void *vp)\n" name;
> + pr "{\n";
> + pr " struct %s_user_data *user_data = vp;\n" name;
> + pr "\n";
> + List.iter (
> + fun { cbname } ->
> + pr " Py_DECREF (user_data->%s);\n" cbname
Oh, good fix. We were not previously incrementing the ref-count of the
Python Callable, and could have ended up deferencing a garbage-collected
object.
> + ) cls;
> + pr " free (user_data);\n";
> + pr "}\n";
> + pr "\n";
> + );
> +
> | Flags n ->
> pr " uint32_t %s_u32;\n" n;
> pr " unsigned int %s; /* really uint32_t */\n" n
> @@ -3911,8 +3930,7 @@ let print_python_binding name { args; ret } =
> pr " int64_t %s_i64;\n" n;
> pr " long long %s; /* really int64_t */\n" n
> | Mutable arg ->
> - pr " PyObject *%s;\n" (List.hd (name_of_arg arg))
> - | Opaque _ -> ()
> + pr " PyObject *%s;\n" (List.hd (c_name_of_arg arg))
I might have also split out the patch for the rename of s/name_of_arg/c_&/
> | Path n ->
> pr " PyObject *py_%s = NULL;\n" n;
> pr " char *%s = NULL;\n" n
> @@ -3935,34 +3953,17 @@ let print_python_binding name { args; ret } =
> ) args;
> pr "\n";
>
> - (* Allocate the parameter. *)
> + (* Allocate the persistent closure user_data. *)
> List.iter (
> function
> - | ArrayAndLen _
> - | Bool _
> - | BytesIn _
> - | BytesPersistIn _
> - | BytesOut _
> - | BytesPersistOut _
> - | Callback _ -> ()
> - | CallbackPersist (n, _) ->
> - pr " %s_data = malloc (sizeof *%s_data);\n" n n;
> - pr " if (%s_data == NULL) {\n" n;
> + | Closure (false, _) -> ()
> + | Closure (true, cls) ->
> + pr " user_data = malloc (sizeof *user_data);\n";
> + pr " if (user_data == NULL) {\n";
> pr " PyErr_NoMemory ();\n";
> pr " return NULL;\n";
> pr " }\n"
Memory leak. If Closure contains two callbacks, and the first malloc()
succeeds while the second fails, we are not reclaiming the first.
Should our generated python code take advantage of
__attribute__((cleanup)) to make it easier to write?
> @@ -4017,6 +4017,20 @@ let print_python_binding name { args; ret } =
> pr "))\n";
> pr " return NULL;\n";
>
> + (* If the parameter has persistent closures then we need to
> + * make sure the ref count remains positive.
> + *)
> + List.iter (
> + function
> + | Closure (false, _) -> ()
> + | Closure (true, cls) ->
> + List.iter (
> + fun { cbname } ->
> + pr " Py_INCREF (user_data->%s);\n" cbname
> + ) cls
> + | _ -> ()
> + ) args;
> +
Any reason this loop is a separate pass, rather than...
> pr " h = get_handle (py_h);\n";
> List.iter (
> function
> @@ -4044,19 +4058,20 @@ let print_python_binding name { args; ret } =
> pr " %s = malloc (%s);\n" n count
> | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
> pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n
> - | Callback (n, _) | CallbackPersist (n, _) ->
> - pr " if (!PyCallable_Check (%s_data->fn)) {\n" n;
> - pr " PyErr_SetString (PyExc_TypeError,\n";
> - pr " \"callback parameter %s is not callable\");\n"
> - n;
> - pr " return NULL;\n";
> - pr " }\n"
> + | Closure (_, cls) ->
> + List.iter (
> + fun { cbname } ->
> + pr " if (!PyCallable_Check (user_data->%s)) {\n" cbname;
> + pr " PyErr_SetString (PyExc_TypeError,\n";
> + pr " \"callback parameter %s is not callable\");\n" cbname;
> + pr " return NULL;\n";
> + pr " }\n"
> + ) cls
...done here? Also, you have a reference leaks: if the python code
passes a non-Callable, you fail to DECREF it (made trickier when the
Closure has two callbacks, and only the second callback is not Callable).
> @@ -4302,28 +4319,30 @@ class NBD (object):
>
> List.iter (
> fun (name, { args; shortdesc; longdesc }) ->
> - let args = List.map (
> - function
> - | ArrayAndLen (UInt32 n, _) -> n, None
> - | ArrayAndLen _ -> assert false
> - | UInt32 n -> n, None
> - | UInt64 n -> n, None
> - ) args in
> + let args =
> + List.map (
> + function
> + | ArrayAndLen (UInt32 n, _) -> [n, None]
> + | ArrayAndLen _ -> assert false
> + | Bool n -> [n, None]
> + | BytesIn (n, _) | BytesPersistIn (n, _) -> [n, None]
> + | BytesPersistOut (n, _) -> [n, None]
> + | BytesOut (_, count) -> [count, None]
> + | Closure (_, cls) ->
> + List.map (fun { cbname } -> cbname, None) cls
Nicer than my attempt.
> +++ b/generator/states-reply-simple.c
> @@ -64,7 +64,7 @@
> int error = 0;
>
> assert (cmd->error == 0);
> - if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data, cmd->count,
> + if (cmd->cb.fn.read (cmd->cb.user_data, cmd->data, cmd->count,
And here's an example of the rename churn that might be worth a separate
patch.
> +++ b/lib/internal.h
> @@ -212,7 +212,7 @@ struct meta_context {
> struct close_callback {
> struct close_callback *next; /* Linked list. */
> nbd_close_callback cb; /* Function. */
> - void *data; /* Data. */
> + void *user_data; /* Data. */
> };
>
> struct socket_ops {
> @@ -241,14 +241,15 @@ struct socket {
> const struct socket_ops *ops;
> };
>
> -typedef int (*extent_fn) (void *data, const char *metacontext, uint64_t offset,
> +typedef int (*extent_fn) (void *user_data,
> + const char *metacontext, uint64_t offset,
> uint32_t *entries, size_t nr_entries, int *error);
> -typedef int (*read_fn) (void *data, const void *buf, size_t count,
> +typedef int (*read_fn) (void *user_data, const void *buf, size_t count,
> uint64_t offset, int status, int *error);
> -typedef int (*callback_fn) (void *data, int64_t cookie, int *error);
> +typedef int (*callback_fn) (void *user_data, int64_t cookie, int *error);
>
> struct command_cb {
> - void *opaque;
> + void *user_data;
since none of the renames here are necessitated by the generator
changes, but rather make things more consistent.
> +++ b/python/t/405-pread-structured.py
> @@ -24,28 +24,30 @@ h.connect_command (["nbdkit", "-s", "--exit-with-parent", "-v",
>
>
> -def f (data, buf2, offset, s, err):
> +def f (user_data, buf2, offset, s, err):
> assert err.value == 0
> err.value = errno.EPROTO
> - assert data == 42
> + assert user_data == 42
> assert buf2 == expected
> assert offset == 0
> assert s == nbd.READ_DATA
>
> -buf = h.pread_structured (512, 0, 42, f)
> +buf = h.pread_structured (512, 0, lambda *args: f (42, *args))
Nice.
We still need to add coverage of h.aio_pread_structured_callback, to
prove that this actually did what we wanted when two callbacks are
present (the test will fail without this patch).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
-------------- 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/20190716/6e25da89/attachment.sig>
More information about the Libguestfs
mailing list