search for: libnbd_callback_free

Mechanical change: Wherever we call any closure with the LIBNBD_CALLBACK_FREE function, we also call nbd_internal_free_callback with the closure's user_data. This allows calls to associate a free callback with any closure via its user_data pointer. --- generator/states-reply-simple.c | 1 + generator/states-reply-structured.c | 24 ++++++++++++++++++------ generat...

...+++ b/lib/aio.c > @@ -90,6 +90,17 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h, > else > h->cmds_done = cmd->next; > > + /* Free the callbacks. */ > + if (cmd->type != NBD_CMD_READ && cmd->cb.fn.extent) > + 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); > + if (cmd->c...

...le.c +++ b/generator/states-reply-simple.c @@ -60,16 +60,16 @@ case 0: /* guaranteed by START */ assert (cmd); - if (cmd->cb.fn.read) { + if (cmd->cb.fn.chunk) { int error = 0; assert (cmd->error == 0); - if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, - cmd->cb.fn_user_data, - cmd->data, cmd->count, - cmd->offset, LIBNBD_READ_DATA, &error) == -1) + if (cmd->cb.fn.chunk (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, + cm...

...ks 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: - -=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. - -=back - -For example C<nbd_set_debug_callback> sets up a callback which you -could define like...

...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. --- TODO | 2 - docs/libnbd.pod | 72 +++++---------- examples/glib-main-loop.c | 14 +-- examples/strict-structured-reads.c | 65 ++++++-------- generator/g...

As proposed here: https://www.redhat.com/archives/libguestfs/2019-August/msg00130.html I didn't actually read Eric's replies to that yet because I've been concentrating on writing these patches all day. Anyway here they are and I'll look at what Eric said about the proposal next. Rich.

...19 @@ /* 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 */ { REPLY.STRUCTURED_REPLY.START: /* We've only read the simple_reply. The structured_reply is longer, @@ -293,16 +306,19 @@ } if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read) {...

Patches 1 & 2 are rather complex, but the end result is that we pass closures + user_data + free function in single struct parameters as I described previously in this email: https://www.redhat.com/archives/libguestfs/2019-August/msg00210.html Patch 3 adds a convenient FREE_CALLBACK macro which seems a worthwhile simplification if you buy into 1 & 2. Patch 4 adds another macro which is

...gt; 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_...

...e 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. &g...

...generator/states-reply-simple.c @@ -60,16 +60,17 @@ case 0: /* guaranteed by START */ assert (cmd); - if (cmd->cb.fn.chunk) { + if (cmd->cb.fn.chunk.callback) { int error = 0; assert (cmd->error == 0); - if (cmd->cb.fn.chunk (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, - cmd->cb.fn_user_data, - cmd->data, cmd->count, - cmd->offset, LIBNBD_READ_DATA, &error) == -1) + if (cmd->cb.fn.chunk.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, +...

I think I've addressed everything that was raised in review. Some of the highlights: - Callbacks should be freed reliably along all exit paths. - There's a simple test of closure lifetimes. - I've tried to use VALID|FREE in all the places where I'm confident that it's safe and correct to do. There may be more places. Note this is an optimization and shouldn't

...crement 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 poss...

...g_callback ( struct nbd_handle *h, int (*debug_fn) (int valid_flag, // <-- note void *user_data, const char *context, const char *msg), void *user_data); The extra ‘valid_flag’ contains one or both of LIBNBD_CALLBACK_VALID and LIBNBD_CALLBACK_FREE. Callback code would look like: int my_debug_fn (int valid_flag, void *user_data, const char *context, const char *msg) { if (valid_flag & LIBNBD_CALLBACK_VALID) { // This is the normal callback as before. printf ("debug msg = %s\n", msg);...

...Our mails are crossing; I had typed this up (so I'm sending now) even though I already see that you've tweaked things in v2. I've got two threads of thoughts below. > > (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.) [1] In my previous reply, I assumed that the nbd_aio_FOO_callback function would use this paradigm... > +++ b/examples/strict-structured-reads.c > @@ -127,11 +131,14 @@ read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset,...

...crement 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 poss...

.....c1decb2 100644 --- a/lib/debug.c +++ b/lib/debug.c @@ -38,13 +38,24 @@ nbd_unlocked_get_debug (struct nbd_handle *h) return h->debug; } +int +nbd_unlocked_clear_debug_callback (struct nbd_handle *h) +{ + if (h->debug_callback) + /* ignore return value */ + h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL); + h->debug_callback = NULL; + h->debug_data = NULL; + return 0; +} + int nbd_unlocked_set_debug_callback (struct nbd_handle *h, nbd_debug_callback debug_callback, void *data) { - if (h->debug_callback) - /* ignore...

...ndenting 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"...

...crement 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 poss...

This implements most of what I wrote here: https://www.redhat.com/archives/libguestfs/2019-July/msg00213.html