[Libguestfs] [PATCH libnbd 2/3] lib: Implement closure lifetimes.
Eric Blake
eblake at redhat.com
Wed Jul 24 15:18:20 UTC 2019
On 7/24/19 7:17 AM, Richard W.M. Jones wrote:
> Previously closures had a crude flag which tells if they are
> persistent or transient. Transient closures (flag = false) last for
> the lifetime of the currently called libnbd function. Persistent
> closures had an indefinite lifetime which could last for as long as
> the handle. In language bindings handling persistent closures was
> wasteful as we needed to register a "close callback" to free the
> closure when the handle is closed. But if you had submitted thousands
> of asynchronous commands you would end up registering thousands of
> close callbacks.
>
> We actually have far more information about the lifetime of closures.
> We know precisely when they will no longer be needed by the library.
> Callers can use this information to free up associated user data
> earlier. In particular in language bindings we can remove roots /
> decrement reference counts at the right place to free the closure,
> without waiting for the handle to be closed.
>
> The solution to this is to introduce the concept of a closure
> lifetime. The callback is called with an extra valid_flag parameter
> which is a bitmap containing LIBNBD_CALLBACK_VALID and/or
> LIBNBD_CALLBACK_FREE. LIBNBD_CALLBACK_VALID corresponds to a normal
> call of the callback function by the library. After the library has
> finished with the callback (declaring that this callback will never be
> needed or called again), it is called once more with
> valid_flag == LIBNBD_CALLBACK_FREE.
>
> (Note it is also possible for the library to call the callback with
> valid_flag == LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, meaning it's the
> last valid call.)
>
>
> diff --git a/docs/libnbd.pod b/docs/libnbd.pod
> index 631bb3b..e9a6030 100644
> --- a/docs/libnbd.pod
> +++ b/docs/libnbd.pod
> @@ -487,7 +487,59 @@ 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
> -as the first parameter to the callback.
> +as the second parameter to the callback.
> +
> +=head2 Callback lifetimes
> +
> +All callbacks have an C<int valid_flag> parameter which is used to
> +help with the lifetime of the callback. C<valid_flag> contains the
> +I<logical or> of:
Again, worth mentioning that this explicitly only for the C bindings?
> +
> +=over 4
> +
> +=item C<LIBNBD_CALLBACK_VALID>
> +
> +The callback parameters are valid and this is a normal callback.
> +
> +=item C<LIBNBD_CALLBACK_FREE>
> +
> +This is the last time the library will call this function. Any data
> +associated with the callback can be freed.
> +
> +=item other bits
> +
> +Other bits in C<valid_flag> should be ignored.
Are we guaranteeing never to set other bits, or that other bits will be
0 for now and only set later if the client opts in to whatever the new
bits are useful for? But I'm okay with leaving this sentence as worded.
> +
> +=back
> +
> +For example C<nbd_set_debug_callback> sets up a callback which you
> +could define like this:
> +
> + int my_debug_fn (int valid_flag, void *user_data,
> + const char *context, const char *msg)
> + {
> + if (valid_flag & LIBNBD_CALLBACK_VALID) {
> + printf ("context = %s, msg = %s\n", context, msg);
> + }
> + if (valid_flag & LIBNBD_CALLBACK_FREE) {
> + /* If you need to free something, for example: */
> + free (user_data);
> + }
> + return 0;
> + }
> +
> +If you don't care about the freeing feature then it is also correct to
> +write this:
> +
> + int my_debug_fn (int valid_flag, void *user_data,
> + const char *context, const char *msg)
> + {
> + if (!(valid_flag & LIBNBD_CALLBACK_VALID)) return 0;
> +
> + // Rest of callback as normal.
> + }
Both examples are good.
> +++ b/examples/strict-structured-reads.c
> @@ -51,12 +51,16 @@ static int64_t total_bytes;
> static int total_success;
>
> static int
> -read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset,
> +read_chunk (int valid_flag, void *opaque,
> + const void *bufv, size_t count, uint64_t offset,
> int status, int *error)
> {
> struct data *data = opaque;
> struct range *r, **prev;
>
> + if (!(valid_flag & LIBNBD_CALLBACK_VALID))
> + return 0;
> +
This one is okay (since we pass the same opaque to both this function
and the later callback function, so have nothing to free at the last use
of this one)...
> /* libnbd guarantees this: */
> assert (offset >= data->offset);
> assert (offset + count <= data->offset + data->count);
> @@ -127,11 +131,14 @@ read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset,
> }
>
> static int
> -read_verify (void *opaque, int64_t cookie, int *error)
> +read_verify (int valid_flag, void *opaque, int64_t cookie, int *error)
> {
> struct data *data = opaque;
> int ret = -1;
>
> + if (!(valid_flag & LIBNBD_CALLBACK_VALID))
> + return 0;
...but this one is wrong; it calls free(data) which should be deferred
to the valid_flag & LIBNBD_CALLBACK_FREE portion. (It looks like later
on in the patch, I see that the nbd_aio_FOO_callback callback is always
called exactly once with VALID|FREE, so your bug here is masked because
of that calling convention - but we should really fix this one to be a
better example).
> +++ b/generator/generator
> @@ -849,10 +849,7 @@ and arg =
> written by the function *)
> | BytesPersistIn of string * string (* same as above, but buffer persists *)
> | BytesPersistOut of string * string
> -| 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 *)
> +| Closure of closure (* function pointer + void *opaque *)
Re-echoing my comment in 1/3, writing this as 'Closure of string * arg
list' may seem cleaner (certainly less typing below).
> @@ -3196,6 +3180,12 @@ let rec print_arg_list ?(handle = false) ?(user_data = false)
> if types then pr "struct nbd_handle *";
> pr "h"
> );
> + if valid_flag then (
> + if !comma then pr ", ";
> + comma := true;
> + if types then pr "int ";
> + pr "valid_flag";
Should this be 'unsigned int valid_flag', as bitmasks over signed values
have awkward semantics if you touch the sign bit? But I'm okay with int
for now, as it matches 'int nbd_aio_get_direction(...)' and
nbd_pread_structured using 'int status' in its callback.
> @@ -3228,10 +3218,10 @@ let rec print_arg_list ?(handle = false) ?(user_data = false)
> pr "%s, " n;
> if types then pr "size_t ";
> pr "%s" len
> - | Closure (_, { cbname; cbargs }) ->
> + | Closure { cbname; cbargs } ->
> if types then (
> pr "int (*%s) " cbname;
> - print_arg_list ~user_data:true cbargs;
> + print_arg_list ~valid_flag:true ~user_data:true cbargs;
Indentation.
> + | 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_arg_list ~user_data:true cbargs;
> + C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
> pr "\n";
> pr "{\n";
> + pr " if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
> pr " int ret;\n";
> pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
Do we care about the generated indentation looking sane? (That can be a
followup patch, to minimize the churn on this one...)
> pr " PyObject *py_args, *py_ret;\n";
> @@ -3927,6 +3907,12 @@ let print_python_binding name { args; ret } =
> | UInt _ | UInt32 _ -> assert false
> ) cbargs;
> pr " return ret;\n";
> + pr " }\n";
Ouch. Calling 'return ret' before checking for LIBNBD_CALLBACK_FREE is
liable to leak. I think you have to defer the return...
> + pr "\n";
> + pr " if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
> + pr " Py_DECREF ((PyObject *)user_data);\n";
> + pr "\n";
> + pr " return 0;\n";
...to here.
> @@ -4048,13 +4034,12 @@ let print_python_binding name { args; ret } =
> pr "))\n";
> pr " return NULL;\n";
>
> - (* If the parameter has persistent closures then we need to
> + (* If the parameter has closures then we need to
> * make sure the ref count remains positive.
> *)
> List.iter (
> function
> - | Closure (false, _) -> ()
> - | Closure (true, { cbname }) ->
> + | Closure { cbname } ->
Now that this match...
> pr " Py_INCREF (%s_user_data);\n" cbname
> | _ -> ()
> ) args;
> @@ -4086,7 +4071,7 @@ 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
> - | Closure (_, { cbname }) ->
> + | Closure { 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;
...and this match are identical, should we group the code?
> +++ b/generator/states-reply-simple.c
I got this far in my review; I'll resume later...
--
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/4cb4f51f/attachment.sig>
More information about the Libguestfs
mailing list