8:03 a.m.
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