search for: valid_flag

The freeing feature can now be done by associating a free callback with a closure's user_data, so having the valid_flag is no longer necessary and it can be completely removed. This mostly reverts commit 2d9b98e96772e282d51dafac07f16387dadc8afa. --- TODO | 2 - docs/libnbd.pod | 64 ++-------------- examples/glib-main-loop.c | 14 +--- examples/stric...

...rticular in language bindings we can remove roots / > decrement 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 &g...

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

...user data earlier. In particular in language bindings we can remove roots / decrement 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...

...user data earlier. In particular in language bindings we can remove roots / decrement 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...

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

...user data earlier. In particular in language bindings we can remove roots / decrement 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...

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

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.

On Wed, Jul 24, 2019 at 10:18:20AM -0500, Eric Blake wrote: > On 7/24/19 7:17 AM, Richard W.M. Jones wrote: > > +=head2 Callback lifetimes > > + > > +All callbacks have an C<int valid_flag> parameter which is used to > > +help with the lifetime of the callback. C<valid_flag> contains the > > +I<logical or> of: > > Again, worth mentioning that this explicitly only for the C bindings? Yes, I'll add that in another commit on top. > > +=item...

...t but not as invasively. We overload the callback so that it can either be a callback function or a "free function". They are distinguished by an extra flag argument passed to the callback: extern int nbd_set_debug_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 vali...

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

...-complete. The completion callback will be invoked with C<cookie> set -to the same value returned by the original API such as -C<nbd_aio_pread_callback> (in rare cases, it is possible that the -completion callback may fire before the original API has returned). +complete. + When C<valid_flag> includes C<LIBNBD_CALLBACK_VALID>, and the completion callback returns C<1>, the command is automatically retired (there is no need to call C<nbd_aio_command_completed>); for any other diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c index 826651e..05a59e3...

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

Instead of using the valid_flag == FREE mechanism, use a free callback to free each closure root. --- generator/generator | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/generator/generator b/generator/generator index 92ce170..109fad6 100755 --- a/generator/generator +++ b/generator/generator @@ -523...

...finished_read, &buffers[i], 0) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } @@ -399,9 +396,9 @@ read_data (gpointer user_data) /* This callback is called from libnbd when any read command finishes. */ static int -finished_read (unsigned valid_flag, void *vp, int64_t rcookie, int *error) +finished_read (unsigned valid_flag, void *vp, int64_t unused, int *error) { - size_t i; + struct buffer *buffer = vp; if (!(valid_flag & LIBNBD_CALLBACK_VALID)) return 0; @@ -411,19 +408,11 @@ finished_read (unsigned valid_flag, void *vp, in...

...verload the callback so that it can either be a callback function > or a "free function". They are distinguished by an extra flag > argument passed to the callback: > > extern int nbd_set_debug_callback ( > struct nbd_handle *h, > int (*debug_fn) (int valid_flag, // <-- note > void *user_data, > const char *context, const char *msg), > void *user_data); Would the 'valid_flag' be presented to non-C bindings, or is it only needed for C code? At any rate, the idea makes sense as a light...

...he completion callback may fire before the original API has returned). -If the completion callback returns C<1>, the command is automatically -retired (there is no need to call C<nbd_aio_command_completed>); for -any other return value, the command still needs to be retired. +When C<valid_flag> includes C<LIBNBD_CALLBACK_VALID>, and the +completion callback returns C<1>, the command is automatically retired +(there is no need to call C<nbd_aio_command_completed>); for any other +return value, the command still needs to be retired. =head2 Callbacks with C<int *er...

v1 was here: https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00231 The changes address everything that Eric picked up in his review of the first two patches. I have also added two more patches (4 and 5) which respectively fix docs and change int status -> unsigned status, as discussed. Passes make, check, check-valgrind. Rich.

...lugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -57,6 +57,7 @@ /* The per-transaction details */ struct transaction { + int64_t cookie; sem_t sem; uint32_t early_err; uint32_t err; @@ -353,15 +354,19 @@ nbdplug_prepare (struct transaction *trans) } static int -nbdplug_notify (unsigned valid_flag, void *opaque, int64_t cookie, int *error) +nbdplug_notify (unsigned valid_flag, void *opaque, int *error) { struct transaction *trans = opaque; if (!(valid_flag & LIBNBD_CALLBACK_VALID)) return 0; + /* There's a possible race here where trans->cookie has not yet been +...