similar to: [PATCH libnbd v2] lib: Atomically update h->state when leaving the locked region.

Split h->state into: - h->public_state = the state on entry to the locked region This is also the atomically, publicly visible state. - h->state = the real current state of the handle When we leave the locked region we update h->public_state with h->state, so that from outside the lock the handle appears to move atomically from its previous state to the final state without

Split h->state into: - h->state = the state on entry to the locked region - h->next_state = the current state and what the "publicly visible" state will become when we leave the locked region Some calls to get_state become calls to get_next_state depending on which of these they are trying to read. Calls to set_state become set_next_state because that is what gets

All accesses to the h->state field are redirected through these macros (except when we create the handle). This is simple refactoring. --- generator/generator | 18 +++++++++--------- lib/connect.c | 10 +++++----- lib/disconnect.c | 8 ++++---- lib/internal.h | 3 +++ lib/is-state.c | 12 ++++++------ lib/rw.c | 4 ++-- 6 files changed, 29 insertions(+), 26

v1 was here: https://www.redhat.com/archives/libguestfs/2019-June/thread.html#00055 v2 was here: https://www.redhat.com/archives/libguestfs/2019-June/thread.html#00067 v3: - Fix atomicly -> atomically in commit message. - Fix a comment. - Fix TOCTTOU: There is now an inline function generated called <name>_is_permitted_state, and this is called twice, first outside the

On 6/5/19 6:15 AM, Richard W.M. Jones wrote: > Split h->state into: > > - h->state = the state on entry to the locked region > > - h->next_state = the current state and what the "publicly visible" > state will become when we leave the locked region The rest of this thread discusses potential other names, such as h->public_state for the state visible

On 6/5/19 8:03 AM, Richard W.M. Jones wrote: > On Wed, Jun 05, 2019 at 01:35:32PM +0100, Richard W.M. Jones wrote: >> On Wed, Jun 05, 2019 at 12:15:37PM +0100, Richard W.M. Jones wrote: >>> -#define set_state(h,next_state) ((h)->state) = (next_state) >>> +#define set_next_state(h,_next_state) ((h)->next_state) = (_next_state) >>> +#define get_next_state(h)

For each nbd_(unlocked_)?aio_is_* function, split it into an internal function that tests the state and a public visible API function. For calls within the library, use the internal function. This is simple refactoring. As a side effect this fixes a race condition identified by Eric Blake: Thread A Thread B (in a call that holds h->lock) (calling nbd_aio_pread)

On Wed, Jun 05, 2019 at 01:35:32PM +0100, Richard W.M. Jones wrote: > On Wed, Jun 05, 2019 at 12:15:37PM +0100, Richard W.M. Jones wrote: > > -#define set_state(h,next_state) ((h)->state) = (next_state) > > +#define set_next_state(h,_next_state) ((h)->next_state) = (_next_state) > > +#define get_next_state(h) ((h)->next_state) > > #define get_state(h)

On Wed, Jun 05, 2019 at 12:15:37PM +0100, Richard W.M. Jones wrote: > -#define set_state(h,next_state) ((h)->state) = (next_state) > +#define set_next_state(h,_next_state) ((h)->next_state) = (_next_state) > +#define get_next_state(h) ((h)->next_state) > #define get_state(h) ((h)->state) So I wonder if it's better to rename get_state as get_last_state or

I need to think about this patch series a bit more, but it does at least pass the tests. Rich.

On Mon, Jun 12, 2023 at 07:27:53PM +0100, Richard W.M. Jones wrote: > Logging state transitions in debug mode produces huge amounts of > output which is not especially helpful. This change removes this > debugging output. This reduces the debug output by approximately two > thirds. > --- > generator/state_machine_generator.ml | 12 ------------ > 1 file changed, 12

This is the bare minimum needed to allow the user to take control over the rest of option negotiating. This patch adds several new API: nbd_set_opt_mode() - called during Created to enable the new mode nbd_get_opt_mode() - query whether opt mode is enabled nbd_opt_go() - used in Negotiating state to attempt to use export nbd_opt_abort() - used in Negotiating state to skip Connected state

I think in general we should have a good idea of the thread model that we support with libnbd. At the moment we have a handle lock which only means that simultaneous calls into the handle won't corrupt it; however it doesn't mean that you don't expect the handle state to change between calls if the handle is accessible by two threads. Is that the model we trying to achieve or

On 6/5/19 6:15 AM, Richard W.M. Jones wrote: > For each nbd_(unlocked_)?aio_is_* function, split it into an internal > function that tests the state and a public visible API function. > > For calls within the library, use the internal function. > > This is simple refactoring. > > As a side effect this fixes a race condition identified by Eric Blake: > > Thread A

Place the tracing of return values after any generated 'out:' label, as it is important to see even the errors caused by calling a function in the wrong handle state. Fixes: 9df088bfa9 --- Multiple bugs found today, but only time to submit one patch so far. I found them all while trying to implement .list_exports in nbdkit. Bug 1: 'nbdinfo --list --json $uri' produces invalid

Once the client has requested NBD_CMD_DISC, the protocol states that it must not send any further information to the server (further writes may still be needed for a clean TLS shutdown, but that's a different matter requiring more states). Our state machine can prevent some of this if we have moved to CLOSED, but that's not foolproof because we can queue commands that can't be written

On Fri, Sep 11, 2020 at 09:31:11AM -0500, Eric Blake wrote: > As mentioned in commits 176fc4ea and 609c25f0, our original plan in > adding a flags argument to nbd_shutdown was to let us specify > different behaviors at the libnbd level, rather than NBD protocol > flags (for that, the user has nbd_aio_disconnect). But when we later > parameterized OFlags to accept various bitmasks

The return value is a bitmask, and is already documented as never failing, hence it makes sense to return an unsigned value to make it easier to do bitwise operations without worrying about potential undefined C semantics on a signed value. --- I'm not sure if we want this, or even if we do if my OCaml was the most concise, but it matches the sentiment of our recent effort to let the

We can easily demonstrate a memory leak when repeatedly calling nbd_aio_pread from nbdsh in the wrong state. True, that isn't something a sane client would normally do, but it's worth fixing. The culprit: when nbd_aio_pread returns a cookie, we guarantee to clean up the closure, but if we fail early and never scheduled the command, nothing ever cleans up the closure. We could document

As mentioned in commits 176fc4ea and 609c25f0, our original plan in adding a flags argument to nbd_shutdown was to let us specify different behaviors at the libnbd level, rather than NBD protocol flags (for that, the user has nbd_aio_disconnect). But when we later parameterized OFlags to accept various bitmasks (commit f891340b), we failed to mark nbd_shutdown as using a different bitmask than