[Libguestfs] [PATCH libnbd 2/4] api: Add free function and remove valid_flag parameter.
Eric Blake
eblake at redhat.com
Wed Aug 14 13:03:21 UTC 2019
On 8/13/19 5:36 PM, Richard W.M. Jones wrote:
> Change the way that we deal with freeing closures in language
> bindings:
>
> * The valid_flag is removed (mostly reverting
> commit 2d9b98e96772e282d51dafac07f16387dadc8afa).
>
> * An extra ‘free’ parameter is added to all callback structures. This
> is called by the library whenever the closure won't be called again
> (so the user_data can be freed). This is analogous to valid_flag ==
> LIBNBD_CALLBACK_FREE in old code.
>
> * Language bindings are updated to deal with these changes.
> ---
> +++ b/docs/libnbd.pod
> @@ -620,56 +620,25 @@ because you can use closures to achieve the same effect.
>
> =head2 Callback lifetimes
>
> -All callbacks have an C<unsigned valid_flag> parameter which is used
> -to help with the lifetime of the callback. C<valid_flag> contains the
> -I<logical or> of:
> +You can associate a free function with callbacks. Libnbd will call
maybe 'an optional free function'
> +this function when the callback will not be called again by libnbd.
>
> -=over 4
> +This can be used to free associated C<user_data>. For example:
>
> -=item C<LIBNBD_CALLBACK_VALID>
> + void *my_data = malloc (...);
> +
> + nbd_aio_pread_structured (nbd, buf, sizeof buf, offset,
> + (nbd_chunk_callback) { .callback = my_fn,
> + .user_data = my_data,
> + .free = free },
> + NBD_NULL_CALLBACK(completion),
Needs rebasing based on the tweak to patch 1.
> diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c
> static int
> -read_verify (unsigned valid_flag, void *opaque, int *error)
> +read_verify (void *opaque, int *error)
> {
> int ret = 0;
> + struct data *data = opaque;
>
> - if (valid_flag & LIBNBD_CALLBACK_VALID) {
> - struct data *data = opaque;
> -
> - ret = -1;
> - total_reads++;
> - total_chunks += data->chunks;
> - if (*error)
> - goto cleanup;
> - assert (data->chunks > 0);
> - if (data->flags & LIBNBD_CMD_FLAG_DF) {
> - total_df_reads++;
> - if (data->chunks > 1) {
> - fprintf (stderr, "buggy server: too many chunks for DF flag\n");
> - *error = EPROTO;
> - goto cleanup;
Pre-existing, but:
> - }
> - }
> - if (data->remaining && !*error) {
> - fprintf (stderr, "buggy server: not enough chunks on success\n");
> + ret = -1;
> + total_reads++;
> + total_chunks += data->chunks;
> + if (*error)
> + goto cleanup;
> + assert (data->chunks > 0);
> + if (data->flags & LIBNBD_CMD_FLAG_DF) {
> + total_df_reads++;
> + if (data->chunks > 1) {
> + fprintf (stderr, "buggy server: too many chunks for DF flag\n");
> *error = EPROTO;
> goto cleanup;
> }
> - total_bytes += data->count;
> - total_success++;
> - ret = 0;
> -
> - cleanup:
> - while (data->remaining) {
> - struct range *r = data->remaining;
> - data->remaining = r->next;
> - free (r);
> - }
I half wonder if this cleanup: label should have been part of the FREE
flag...
> }
> + if (data->remaining && !*error) {
> + fprintf (stderr, "buggy server: not enough chunks on success\n");
> + *error = EPROTO;
> + goto cleanup;
> + }
> + total_bytes += data->count;
> + total_success++;
> + ret = 0;
>
> - if (valid_flag & LIBNBD_CALLBACK_FREE)
> - free (opaque);
> + cleanup:
> + while (data->remaining) {
> + struct range *r = data->remaining;
> + data->remaining = r->next;
> + free (r);
> + }
...in which case this loop should be moved into a dedicated free function...
>
> return ret;
> }
> @@ -237,7 +228,7 @@ main (int argc, char *argv[])
> .remaining = r, };
> if (nbd_aio_pread_structured (nbd, buf, sizeof buf, offset,
> (nbd_chunk_callback) { .callback = read_chunk, .user_data = d },
> - (nbd_completion_callback) { .callback = read_verify, .user_data = d },
> + (nbd_completion_callback) { .callback = read_verify, .user_data = d, .free = free
...rather than using free(). But it works as written (mainly because
the completion callback is called exactly once), so I'm fine leaving
this one as you have it.
> @@ -3340,6 +3335,7 @@ let print_closure_structs () =
> pr " int (*callback) ";
> print_cbarg_list cbargs;
> pr ";\n";
> + pr " void (*free) (void *user_data);\n";
> pr "} nbd_%s_callback;\n" cbname;
Minor rebasing needed here too; I'm assuming that you've already done
most of the rebasing work by the time you read this, so I'll quit
pointing it out.
> +++ b/generator/states-reply-structured.c
> @@ -18,17 +18,6 @@
>
> /* State machine for parsing structured replies from the server. */
>
> -static unsigned
> -valid_flags (struct nbd_handle *h)
> -{
> - unsigned valid = LIBNBD_CALLBACK_VALID;
> - uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
> -
> - if (flags & NBD_REPLY_FLAG_DONE)
> - valid |= LIBNBD_CALLBACK_FREE;
> - return valid;
> -}
> -
> /*----- End of prologue. -----*/
>
> /* STATE MACHINE */ {
> @@ -306,20 +295,23 @@ valid_flags (struct nbd_handle *h)
> }
> if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) {
> int scratch = error;
> - unsigned valid = valid_flags (h);
> + uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
>
> /* Different from successful reads: inform the callback about the
> * 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.chunk.callback (valid, cmd->cb.fn.chunk.user_data,
> + if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data,
> cmd->data + (offset - cmd->offset),
> 0, offset, LIBNBD_READ_ERROR,
> &scratch) == -1)
> if (cmd->error == 0)
> cmd->error = scratch;
> - if (valid & LIBNBD_CALLBACK_FREE)
> + if (flags & NBD_REPLY_FLAG_DONE) {
> + if (cmd->cb.fn.chunk.free)
> + cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data);
> cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */
> + }
In the old code, we tried as hard as possible to favor a single
callback(VALID|FREE) to reduce the number of callbacks being made. But
with a split callback function, I no longer see any advantage to using
the free() callback as soon as possible; it may be easier to ONLY call
the free() callback when retiring a command. That would simplify
multiple spots in this file. However, I could also see keeping this
patch as mechanical as possible, and doing that cleanup as a followup.
> +++ b/lib/aio.c
> @@ -32,18 +32,18 @@ void
> nbd_internal_retire_and_free_command (struct command *cmd)
> {
> /* Free the callbacks. */
> - if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent.callback)
> - cmd->cb.fn.extent.callback (LIBNBD_CALLBACK_FREE,
> - cmd->cb.fn.extent.user_data,
> - NULL, 0, NULL, 0, NULL);
> - if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback)
> - cmd->cb.fn.chunk.callback (LIBNBD_CALLBACK_FREE,
> - cmd->cb.fn.chunk.user_data,
> - NULL, 0, 0, 0, NULL);
> - if (cmd->cb.completion.callback)
> - cmd->cb.completion.callback (LIBNBD_CALLBACK_FREE,
> - cmd->cb.completion.user_data,
> - NULL);
> + if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent.callback) {
> + if (cmd->cb.fn.extent.free)
> + cmd->cb.fn.extent.free (cmd->cb.fn.extent.user_data);
> + }
> + if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) {
> + if (cmd->cb.fn.chunk.free)
> + cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data);
> + }
> + if (cmd->cb.completion.callback) {
> + if (cmd->cb.completion.free)
> + cmd->cb.completion.free (cmd->cb.completion.user_data);
> + }
If we consolidate callback freeing to JUST this function, it cleans up
all the other spots in generator/* that were calling .free() and setting
.callback=NULL as soon as possible. It also means we are less likely to
need patch 3 as a convenience macro to make the pattern of freeing a
closure easier to type, because we aren't repeating the pattern.
> +++ b/tests/closure-lifetimes.c
> @@ -38,50 +38,58 @@ static char *nbdkit_delay[] =
> "delay-read=10",
> NULL };
>
> -static unsigned debug_fn_valid;
> -static unsigned debug_fn_free;
> -static unsigned read_cb_valid;
> -static unsigned read_cb_free;
> -static unsigned completion_cb_valid;
> -static unsigned completion_cb_free;
> +static unsigned debug_fn_called;
> +static unsigned debug_fn_freed;
> +static unsigned read_cb_called;
> +static unsigned read_cb_freed;
> +static unsigned completion_cb_called;
> +static unsigned completion_cb_freed;
>
> static int
> -debug_fn (unsigned valid_flag, void *opaque,
> - const char *context, const char *msg)
> +debug_fn (void *opaque, const char *context, const char *msg)
> {
> - if (valid_flag & LIBNBD_CALLBACK_VALID)
> - debug_fn_valid++;
> - if (valid_flag & LIBNBD_CALLBACK_FREE)
> - debug_fn_free++;
> + debug_fn_called++;
> return 0;
Here, we could add: assert(!debug_fn_freed)
> }
>
> +static void
> +debug_fn_free (void *opaque)
> +{
> + debug_fn_freed++;
> +}
Maybe even here, too (although if we later assert debug_fn_freed == 1,
it's the same effect).
Overall, I think this is a good patch. I'm okay if you want to push
your rebased version now, whether or not you save the consolidation of
.free() into just retirement as a followup.
--
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/20190814/268cc08c/attachment.sig>
More information about the Libguestfs
mailing list