search for: valid_flags

...a/generator/states-reply-structured.c b/generator/states-reply-structured.c index b016cd7..846cc35 100644 --- a/generator/states-reply-structured.c +++ 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 */ { @@...

On 7/24/19 7:17 AM, Richard W.M. Jones wrote: > Previously closures had a crude flag which tells if they are > persistent or transient. Transient closures (flag = false) last for > the lifetime of the currently called libnbd function. Persistent > closures had an indefinite lifetime which could last for as long as > the handle. In language bindings handling persistent closures

...a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 92d6b5f..7c4d63e 100644 --- a/generator/states-reply-structured.c +++ 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 */ { @@...

Previously closures had a crude flag which tells if they are persistent or transient. Transient closures (flag = false) last for the lifetime of the currently called libnbd function. Persistent closures had an indefinite lifetime which could last for as long as the handle. In language bindings handling persistent closures was wasteful as we needed to register a "close callback" to

Previously closures had a crude flag which tells if they are persistent or transient. Transient closures (flag = false) last for the lifetime of the currently called libnbd function. Persistent closures had an indefinite lifetime which could last for as long as the handle. In language bindings handling persistent closures was wasteful as we needed to register a "close callback" to

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

Previously closures had a crude flag which tells if they are persistent or transient. Transient closures (flag = false) last for the lifetime of the currently called libnbd function. Persistent closures had an indefinite lifetime which could last for as long as the handle. In language bindings handling persistent closures was wasteful as we needed to register a "close callback" to

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

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

On Mon, Jul 22, 2019 at 10:08:25AM +0100, Richard W.M. Jones wrote: > On Sat, Jul 20, 2019 at 07:38:45AM +0100, Richard W.M. Jones wrote: > > More thoughts on callbacks, etc. following on from: > > https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00184 > > > > Closure lifetimes > > ----------------- Here's a possibly better idea which still

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

As discussed in this thread, the parameter is an invitation to write code with race conditions: https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00309 --- docs/libnbd.pod | 6 +- examples/glib-main-loop.c | 10 ++-- examples/strict-structured-reads.c | 2 +- generator/generator | 57

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 @@ -5239,9 +5239,6 @@ let print_ocaml_binding (name, { args; optargs; ret })

There was a theoretic race in this example: If the server was very fast at handling commands then it's possible that in a call such as: cookie = nbd_aio_pread_callback (..., callback, ...); buffers[i].cookie = cookie; nbd_aio_pread_callback finished and calls the callback before returning. buffers[i].cookie would therefore not be set, but the callback() function was checking the list of

On 7/22/19 6:50 AM, Richard W.M. Jones wrote: > On Mon, Jul 22, 2019 at 10:08:25AM +0100, Richard W.M. Jones wrote: >> On Sat, Jul 20, 2019 at 07:38:45AM +0100, Richard W.M. Jones wrote: >>> More thoughts on callbacks, etc. following on from: >>> https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00184 >>> >>> Closure lifetimes

On 7/25/19 8:07 AM, Richard W.M. Jones wrote: > Previously closures had a crude flag which tells if they are > persistent or transient. Transient closures (flag = false) last for > the lifetime of the currently called libnbd function. Persistent > closures had an indefinite lifetime which could last for as long as > the handle. In language bindings handling persistent closures

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.

--- configure.ac | 4 ++-- plugins/nbd/nbd.c | 12 +++++++++--- README | 2 +- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/configure.ac b/configure.ac index 8c7ee8b..0ce78c7 100644 --- a/configure.ac +++ b/configure.ac @@ -718,12 +718,12 @@ AC_ARG_WITH([libnbd], [], [with_libnbd=check]) AS_IF([test "$with_libnbd" != "no"],[ -