[Libguestfs] [PATCH libnbd v3 1/2] lib: Implement closure lifetimes.
Eric Blake
eblake at redhat.com
Thu Jul 25 15:43:05 UTC 2019
On 7/25/19 8:07 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.
>
> +++ b/.gitignore
> @@ -108,6 +108,7 @@ Makefile.in
> /tests/can-trim-flag
> /tests/can-not-trim-flag
> /tests/can-zero-flag
> +/tests/closure-lifetimes
Always good to see testsuite coverage of new features :) I guess I'll
see below if I can offer more ideas for what the test can do.
> +
> +The valid flag is only present in the C API. It is not needed when
> +using garbage-collected programming languages.
> +
> +=head2 Callbacks and locking
>
> The callbacks are invoked at a point where the libnbd lock is held; as
> such, it is unsafe for the callback to call any C<nbd_*> APIs on the
Missing a change to the auto-retire text and error handling text (since
we have places where we now ignore the return value when calling with
only FREE); it may be worth squashing in:
diff --git i/docs/libnbd.pod w/docs/libnbd.pod
index 4d31c64..1fcbfd7 100644
--- i/docs/libnbd.pod
+++ w/docs/libnbd.pod
@@ -501,9 +501,10 @@ complete. The completion callback will be invoked
with C<cookie> set
to the same value returned by the original API such as
C<nbd_aio_pread_callback> (in rare cases, it is possible that the
completion callback may fire before the original API has returned).
-If the completion callback returns C<1>, the command is automatically
-retired (there is no need to call C<nbd_aio_command_completed>); for
-any other return value, the command still needs to be retired.
+When C<valid_flag> includes C<LIBNBD_CALLBACK_VALID>, and the
+completion callback returns C<1>, the command is automatically retired
+(there is no need to call C<nbd_aio_command_completed>); for any other
+return value, the command still needs to be retired.
=head2 Callbacks with C<int *error> parameter
@@ -515,8 +516,9 @@ all of the completion callbacks, include a parameter
C<error>
containing the value of any error detected so far; if the callback
function fails, it should assign back into C<error> and return C<-1>
to change the resulting error of the overall command. Assignments
-into C<error> are ignored for any other return value; similarly,
+into C<error> are ignored for any other return value or when
+C<valid_flag> did not contain C<LIBNBD_CALLBACK_VALID>; similarly,
assigning C<0> into C<error> does not have an effect.
=head1 SEE ALSO
> +++ b/examples/strict-structured-reads.c
> static int
> -read_verify (void *opaque, int64_t cookie, int *error)
> +read_verify (unsigned valid_flag, void *opaque, int64_t cookie, int *error)
> {
> + int ret = 0;
> +
> + if (valid_flag & LIBNBD_CALLBACK_VALID) {
> struct data *data = opaque;
> - int ret = -1;
>
> + ret = -1;
> total_reads++;
Was this patch posted with whitespace changes ignored? Otherwise, it
looks like you missed reindenting things.
> total_chunks += data->chunks;
> if (*error)
> @@ -160,7 +167,11 @@ read_verify (void *opaque, int64_t cookie, int *error)
> data->remaining = r->next;
> free (r);
> }
> - free (data);
> + }
> +
> + if (valid_flag & LIBNBD_CALLBACK_FREE)
> + free (opaque);
> +
> return ret;
> }
>
> diff --git a/generator/generator b/generator/generator
> @@ -1350,11 +1346,10 @@ protocol extensions).";
> "pread_structured", {
> default_call with
> args = [ BytesOut ("buf", "count"); UInt64 "offset";
> - Closure (false,
> - { cbname="chunk";
> + Closure { cbname="chunk";
> cbargs=[BytesIn ("subbuf", "count");
Indentation.
> UInt64 "offset"; UInt "status";
> - Mutable (Int "error")] });
> + Mutable (Int "error")] };
> Flags "flags" ];
> ret = RErr;
> permitted_states = [ Connected ];
> @@ -1541,13 +1536,12 @@ punching a hole.";
> "block_status", {
> default_call with
> args = [ UInt64 "count"; UInt64 "offset";
> - Closure (false,
> - { cbname="extent";
> + Closure { cbname="extent";
> cbargs=[String "metacontext";
and again; I'll quit pointing it out for args=[].
> @@ -3714,28 +3707,16 @@ let print_python_binding name { args; ret } =
> + | 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 " int ret;\n";
> + pr " int ret = 0;\n";
> + pr "\n";
> + pr " if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
> pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
Another place where indentation looks odd, unless this email was
generated with whitespace changes ignored. (Cleaning up whitespace in a
separate patch to let THIS patch focus on the semantic changes is also
acceptable)
> @@ -4684,16 +4638,24 @@ let print_ocaml_binding (name, { args; ret }) =
> pr "\n";
> 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 " int ret;\n";
> + pr " int ret = 0;\n";
> pr "\n";
> + pr " if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
> pr " caml_leave_blocking_section ();\n";
Same here.
> +++ b/generator/states-reply-structured.c
> @@ -298,7 +298,7 @@
> * current error rather than any earlier one. If the callback fails
> * without setting errno, then use the server's error below.
> */
> - if (cmd->cb.fn.read (cmd->cb.fn_user_data,
> + if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
> cmd->data + (offset - cmd->offset),
> 0, offset, LIBNBD_READ_ERROR, &scratch) == -1)
> if (cmd->error == 0)
We could still optimize this file based on NBD_REPLY_FLAG_DONE, but that
can be a followup.
> @@ -499,7 +499,7 @@
> /* Call the caller's extent function. */
> int error = cmd->error;
>
> - if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
> + if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
> meta_context->name, cmd->offset,
> &h->bs_entries[1], (length-4) / 4, &error) == -1)
> if (cmd->error == 0)
Hmm - no change to the FINISH state, which means you are relying on
command retirement to free chunk/extent instead. As long as that
happens, we should be okay, though.
> diff --git a/generator/states-reply.c b/generator/states-reply.c
> index 6ea43d5..8f62923 100644
> --- a/generator/states-reply.c
> +++ b/generator/states-reply.c
> @@ -170,9 +170,13 @@ save_reply_state (struct nbd_handle *h)
> /* Notify the user */
> if (cmd->cb.callback) {
> int error = cmd->error;
> + int r;
>
> assert (cmd->type != NBD_CMD_DISC);
> - switch (cmd->cb.callback (cmd->cb.user_data, cookie, &error)) {
> + r = cmd->cb.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
> + cmd->cb.user_data, cookie, &error);
> + cmd->cb.callback = NULL; /* because we've freed it */
> + switch (r) {
Moving the side effect out of the switch() condition is a reasonable
move; my fault for putting it there in the first place.
> case -1:
> if (error)
> cmd->error = error;
> @@ -190,7 +194,7 @@ save_reply_state (struct nbd_handle *h)
> h->cmds_in_flight = cmd->next;
> cmd->next = NULL;
> if (retire)
> - free (cmd);
> + nbd_internal_retire_and_free_command (cmd);
Looks like a nice helper function. (Side note: using 'git config
diff.orderfile some/file' can be a way to rearrange patches to be easier
to review logically: I think placing lib/internal.h and
generator/generator first, then generator/* and lib/* prior to tests/*
interop/* examples/* would have have made this review easier to read -
maybe I should propose a scripts/git.orderfile similar to nbdkit).
> +++ b/lib/aio.c
> @@ -27,6 +27,24 @@
>
> #include "internal.h"
>
> +/* Internal function which retires and frees a command. */
> +void
> +nbd_internal_retire_and_free_command (struct command *cmd)
> +{
> + /* Free the callbacks. */
> + if (cmd->type != NBD_CMD_READ && cmd->cb.fn.extent)
Looks odd that this was not spelled 'cmd->type == NBD_CMD_BLOCK_STATUS'.
> + cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
> + NULL, 0, NULL, 0, NULL);
> + if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read)
> + cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
> + NULL, 0, 0, 0, NULL);
Perhaps we could even have:
switch (cmd->type) {
case NBD_CMD_READ:
if (cmd->cb.fn.read) ...
break;
case NBD_CMD_BLOCK_STATUS:
if (cmd->cb.fn.extent) ...
break;
default:
assert (!cmd->cb.fn.read);
}
> + if (cmd->cb.callback)
> + cmd->cb.callback (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
> + 0, NULL);
> +
> + free (cmd);
> +}
But as written, the function operates correctly. So up to you if you
want to tweak it.
> @@ -96,6 +96,10 @@ nbd_close (struct nbd_handle *h)
> if (h == NULL)
> return;
>
> + /* Free user callbacks first. */
> + if (h->debug_fn)
> + h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
> +
> for (cc = h->close_callbacks; cc != NULL; cc = cc_next) {
I recommend either setting h->debug_fn = NULL here, or deferring the
FREE callback to after the h->sock->ops->close (h->sock) below.
Otherwise, a future edit to lib/sockets.c to add in a debug statement
there will cause a use-after-free at a distance.
> +++ b/tests/closure-lifetimes.c
> +static int
> +debug_fn (unsigned valid_flag, void *opaque,
> + const char *context, const char *msg)
> +{
Is it worth assert(!debug_fn_free), to prove that we never have
use-after-free?
> + if (valid_flag & LIBNBD_CALLBACK_VALID)
> + debug_fn_valid++;
> + if (valid_flag & LIBNBD_CALLBACK_FREE)
> + debug_fn_free++;
> + return 0;
> +}
> +
> +static int
> +read_cb (unsigned valid_flag, void *opaque,
> + const void *subbuf, size_t count,
> + uint64_t offset, unsigned status, int *error)
> +{
Same here.
> + if (valid_flag & LIBNBD_CALLBACK_VALID)
> + read_cb_valid++;
> + if (valid_flag & LIBNBD_CALLBACK_FREE)
> + read_cb_free++;
> + return 0;
> +}
> +
> +static int
> +completion_cb (unsigned valid_flag, void *opaque,
> + int64_t cookie, int *error)
> +{
and again
> + if (valid_flag & LIBNBD_CALLBACK_VALID)
> + completion_cb_valid++;
> + if (valid_flag & LIBNBD_CALLBACK_FREE)
> + completion_cb_free++;
> + return 0;
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> + struct nbd_handle *nbd;
> + int64_t cookie;
> + char buf[512];
> +
> + /* Check debug functions are freed when a new debug function is
> + * registered, and when the handle is closed.
> + */
> + nbd = nbd_create ();
> + assert (nbd);
> +
> + nbd_set_debug_callback (nbd, debug_fn, NULL);
> + assert (debug_fn_free == 0);
> +
> + nbd_set_debug_callback (nbd, debug_fn, NULL);
> + assert (debug_fn_free == 1);
If you add asserts against use-after-free above, you'd also need to
reset debug_fn_free back to 0 here.
> +
> + nbd_close (nbd);
> + assert (debug_fn_free == 2);
with knock-on affects to what you assert here.
Is there any way to reliably test whether debug_fn_valid was
incremented? (To some extent, it depends on whether nbd_set_debug was
used, and if we know for sure that we triggered an action that results
in a debug statement). Testing for >0 is sufficient, testing for a
specific value would be fragile as we may add or remove debug calls in
future refactorings.
> +
> + /* Test command callbacks are freed when the command is retired. */
> + nbd = nbd_create ();
> + assert (nbd);
> + assert (nbd_connect_command (nbd, nbdkit) == 0);
Side effects in an assert. Nasty. (Maybe you can get away with it if we
explicitly #undef NDEBUG at the top of this file, so the test still
works if someone does ./configure CFLAGS=-DNDEBUG, but splitting the
side effects from the test validation seems wise)
> +
> + cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0,
> + read_cb, NULL,
> + completion_cb, NULL, 0);
> + assert (read_cb_free == 0);
> + assert (completion_cb_free == 0);
> + while (!nbd_aio_command_completed (nbd, cookie))
> + assert (nbd_poll (nbd, -1) >= 0);
More side effects in an assert :(
> +
> + assert (read_cb_valid == 1);
> + assert (completion_cb_valid == 1);
> + assert (read_cb_free == 1);
> + assert (completion_cb_free == 1);
> +
> + nbd_close (nbd);
> +
> + /* Test command callbacks are freed if the handle is closed without
> + * running the commands.
> + *
> + * Note it's possible that nbd_aio_pread_structured_callback might
> + * actually complete the command if the server is very fast.
We can use --filter=delay delay-read=15 to ensure the server is not very
fast, to reduce the risk of the race (but only for this part of the
test, not the earlier part where we do wait for completion - which means
two separate parameter lists for nbd_connect_command() calls...)
> + */
> + read_cb_valid = read_cb_free =
> + completion_cb_valid = completion_cb_free = 0;
> + nbd = nbd_create ();
> + assert (nbd);
> + assert (nbd_connect_command (nbd, nbdkit) == 0);
side effects :(
> +
> + cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0,
> + read_cb, NULL,
> + completion_cb, NULL, 0);
> + nbd_close (nbd);
> +
> + assert (read_cb_free == 1);
> + assert (completion_cb_free == 1);
Worth asserting that read_cb_valid and completion_cb_valid are 0 (if we
are confident in our ability to strand the command unretired due to use
of a filter delay)?
> +
> + exit (EXIT_SUCCESS);
> +}
> diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c
> index 95e029b..9e88d6b 100644
> --- a/tests/meta-base-allocation.c
Most of my comments can be addressed by followups or pertain to the new
test; I'm okay if you want to push this without posting v4.
--
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/20190725/61f4db3e/attachment.sig>
More information about the Libguestfs
mailing list