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