search for: completion_cb_free

...eneral Public @@ -42,6 +42,8 @@ static unsigned debug_fn_called; static unsigned debug_fn_freed; static unsigned read_cb_called; static unsigned read_cb_freed; +static unsigned block_status_cb_called; +static unsigned block_status_cb_freed; static unsigned completion_cb_called; static unsigned completion_cb_freed; @@ -74,6 +76,21 @@ read_cb_free (void *opaque) read_cb_freed++; } +static int +block_status_cb (void *opaque, const char *meta, uint64_t offset, + uint32_t *entries, size_t nr_entries, int *error) +{ + assert (!block_status_cb_freed); + block_status_cb_called++; + return...

...closure-lifetimes.c @@ -38,50 +38,59 @@ 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, +debug_fn (void *opaque,...

...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 *contex...

As promised in my earlier thread on libnbd completion callback question. Eric Blake (2): generator: Refactor handling of closures in unlocked functions generator: Free closures on failure docs/libnbd.pod | 2 +- generator/C.ml | 48 +++++++++++------ generator/C.mli | 1 + lib/debug.c | 7 +-- lib/opt.c | 31 ++++++-----

This is my proposal for fixing the documentation to match practice (namely, that completion.callback is not invoked in the cases where the aio call itself reports errors); we could instead try to go the other direction and tweak the generator to guarantee that both completion.callback and completion.free are reached no matter what, but that felt more invasive to me. Eric Blake (2): api: Tighten

...or (ESRCH, "no subprocess exists"); diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c index 5a7cd9c..70788b6 100644 --- a/tests/closure-lifetimes.c +++ b/tests/closure-lifetimes.c @@ -143,7 +143,7 @@ main (int argc, char *argv[]) assert (read_cb_freed == 1); assert (completion_cb_freed == 1); - nbd_kill_command (nbd, 0); + nbd_kill_subprocess (nbd, 0); nbd_close (nbd); /* Test command callbacks are freed if the handle is closed without @@ -162,7 +162,7 @@ main (int argc, char *argv[]) .free = completio...

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.

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

...; +} > + > +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 close...

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

...c 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...

...exit-with-parent", "-v", + "null", "size=512", 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 int +debug_fn (unsigned valid_flag, 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++; + return 0; +} + +static int +read_cb (unsigned...

...;, "-v", "--filter=delay", "null", "size=512", - "delay-read=3", + "delay-read=10", NULL }; static unsigned debug_fn_valid; @@ -134,6 +134,7 @@ main (int argc, char *argv[]) assert (read_cb_free == 1); assert (completion_cb_free == 1); + nbd_kill_command (nbd, 0); nbd_close (nbd); /* Test command callbacks are freed if the handle is closed without @@ -149,6 +150,7 @@ main (int argc, char *argv[]) read_cb, NULL, completi...

...sure-lifetimes.c +++ b/tests/closure-lifetimes.c @@ -73,8 +73,7 @@ read_cb (unsigned valid_flag, void *opaque, } static int -completion_cb (unsigned valid_flag, void *opaque, - int64_t cookie, int *error) +completion_cb (unsigned valid_flag, void *opaque, int *error) { assert (completion_cb_free == 0); diff --git a/tests/server-death.c b/tests/server-death.c index 56e7e51..7854527 100644 --- a/tests/server-death.c +++ b/tests/server-death.c @@ -33,7 +33,7 @@ static bool trim_retired; static const char *progname; static int -callback (unsigned valid_flag, void *ignored, int64_t cookie...

...completion_cb, NULL, 0); + cookie = nbd_aio_pread_structured (nbd, buf, sizeof buf, 0, + read_cb, NULL, + completion_cb, NULL, 0); if (cookie == -1) NBD_ERROR; assert (read_cb_free == 0); assert (completion_cb_free == 0); @@ -143,9 +143,9 @@ main (int argc, char *argv[]) if (nbd == NULL) NBD_ERROR; if (nbd_connect_command (nbd, nbdkit_delay) == -1) NBD_ERROR; - cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0, - read_cb, NULL, -...

This applies on top of the OClosure v2 series posted a few minutes ago. Rich.

...0); + (nbd_chunk_callback) { .callback = read_cb }, + (nbd_completion_callback) { .callback = completion_cb }, + 0); if (cookie == -1) NBD_ERROR; assert (read_cb_free == 0); assert (completion_cb_free == 0); @@ -144,8 +145,9 @@ main (int argc, char *argv[]) if (nbd_connect_command (nbd, nbdkit_delay) == -1) NBD_ERROR; cookie = nbd_aio_pread_structured (nbd, buf, sizeof buf, 0, - read_cb, NULL, - completion_cb, NULL,...