search for: get_next_state

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 updated. When we leave the locked region we update h->state. The purpose of this patch is to make it easier to reason about the state in lockless code. --- generator/genera...

...ate 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 going through any intermediate states. Some calls to ‘get_state’ become calls to ‘get_next_state’ if the need the real state. Others which need to see the publicly visible state are changed to ‘get_public_state’. All calls to ‘set_state’ become ‘set_next_state’ because that is the real state that gets updated. The purpose of this patch is to make it easier to reason about the state in lockl...

...ate 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 going through any intermediate states. Some calls to ‘get_state’ become calls to ‘get_next_state’ if the need the real state. Others which need to see the publicly visible state are changed to ‘get_public_state’. All calls to ‘set_state’ become ‘set_next_state’ because that is the real state that gets updated. The purpose of this patch is to make it easier to reason about the state in lockl...

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 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 get_visible_state? And/or rename get_next_state/set_next_state to get_state/set_state? Ideas welcome to make the code clearer. Rich. -- Richard Jones, Vir...

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

...| 9 ++++++--- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/lib/disconnect.c b/lib/disconnect.c index 95e9a37..53de386 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -29,8 +29,9 @@ int nbd_unlocked_shutdown (struct nbd_handle *h) { - if (nbd_internal_is_state_ready (get_next_state (h)) || - nbd_internal_is_state_processing (get_next_state (h))) { + if (!h->disconnect_request && + (nbd_internal_is_state_ready (get_next_state (h)) || + nbd_internal_is_state_processing (get_next_state (h)))) { if (nbd_unlocked_aio_disconnect (h, 0) == -1)...

...M +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) ((h)->state) >> >> So I wonder if it's better to rename get_state as get_last_state or >> get_visible_state? > > Or even get_public_state? get_public_state sounds nice (the state that nbd_connection_state wi...

...rint_trace_leave ret; - pr "\n" - ); + pr "\n"; if !need_out_label then pr " out:\n"; + if may_set_error then ( + print_trace_leave ret; + pr "\n" + ); if is_locked then ( pr " if (h->public_state != get_next_state (h))\n"; pr " h->public_state = get_next_state (h);\n"; -- 2.28.0

...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) ((h)->state) > > So I wonder if it's better to rename get_state as get_last_state or > get_visible_state? Or even get_public_state? > And/or rename get_next_state/set_next_state to > get_state/set_state? > > Ide...

...ed code and h->state for internal while still locked; I trust that once you pick a naming scheme you like, you can redo this patch accordingly. I'll go ahead and review the changes as spelled here to at least see if I can spot any problems. > > 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 updated. > > When we leave the locked region we update h->state. > > The purpose of this patch is to make it easier to reason about the > state in l...

...ator.ml > index 274e290952..52c497646d 100644 > --- a/generator/state_machine_generator.ml > +++ b/generator/state_machine_generator.ml > @@ -380,9 +380,6 @@ let > pr " h, &next, blocked\n"; > pr " );\n"; > pr " if (get_next_state (h) != next) {\n"; > - pr " debug (h, \"transition: %%s -> %%s\",\n"; > - pr " \"%s\",\n" display_name; > - pr " nbd_internal_state_short_string (next));\n"; > pr " set_next_st...

...-1; } + /* If IMMEDIATE, abort any commands that have not yet had any bytes + * sent to the server, so that NBD_CMD_DISC will be first in line. + */ + if (flags & LIBNBD_SHUTDOWN_IMMEDIATE) { + struct command **cmd = &h->cmds_to_issue; + if (!nbd_internal_is_state_ready (get_next_state (h))) { + assert (*cmd); + h->cmds_to_issue_tail = *cmd; + cmd = &((*cmd)->next); + } + nbd_internal_abort_commands (h, cmd); + } + if (!h->disconnect_request && (nbd_internal_is_state_ready (get_next_state (h)) || nbd_internal_is_state_p...

...if (h->disconnect_request) { set_error (EINVAL, "cannot request more commands after NBD_CMD_DISC"); @@ -231,13 +231,11 @@ nbd_internal_command_common (struct nbd_handle *h, h->in_flight++; if (h->cmds_to_issue != NULL) { assert (nbd_internal_is_state_processing (get_next_state (h))); - prev_cmd = h->cmds_to_issue; - while (prev_cmd->next) - prev_cmd = prev_cmd->next; - prev_cmd->next = cmd; + h->cmds_to_issue_tail = h->cmds_to_issue_tail->next = cmd; } else { - h->cmds_to_issue = cmd; + assert (h->cmds_to_issue_tail...

...IATE, abort any commands that have not yet had any bytes > + * sent to the server, so that NBD_CMD_DISC will be first in line. > + */ > + if (flags & LIBNBD_SHUTDOWN_IMMEDIATE) { > + struct command **cmd = &h->cmds_to_issue; > + if (!nbd_internal_is_state_ready (get_next_state (h))) { > + assert (*cmd); > + h->cmds_to_issue_tail = *cmd; > + cmd = &((*cmd)->next); > + } > + nbd_internal_abort_commands (h, cmd); > + } > + > if (!h->disconnect_request && > (nbd_internal_is_state_ready (get_next_...

Noticed while thinking about the recent threads wondering if we need a more efficient lookup from cookie back to command. Both of these fix bugs, but are tricky enough that I'm posting for review. Eric Blake (2): lib: Decrement in_flight at response, not retirement lib: Do O(1) rather than O(n) queue insertion generator/states-issue-command.c | 2 ++ generator/states-reply.c |

...Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -40,7 +40,8 @@ static int error_unless_ready (struct nbd_handle *h) { - if (nbd_internal_is_state_ready (get_next_state (h))) + if (nbd_internal_is_state_ready (get_next_state (h)) || + nbd_internal_is_state_negotiating (get_next_state (h))) return 0; /* Why did it fail? */ diff --git a/lib/is-state.c b/lib/is-state.c index 1a85c7a..e019e53 100644 --- a/lib/is-state.c +++ b/lib/is-state.c @@ -1,5 +1,5...

...back);\n" cbname + | _ -> () + ) args; + List.iter ( + function + | OClosure { cbname } -> + pr " FREE_CALLBACK (%s_callback);\n" cbname + | OFlags _ -> () + ) optargs; if is_locked then ( pr " if (h->public_state != get_next_state (h))\n"; pr " h->public_state = get_next_state (h);\n"; diff --git a/lib/debug.c b/lib/debug.c index 1b503d9..b598ad3 100644 --- a/lib/debug.c +++ b/lib/debug.c @@ -54,6 +54,7 @@ nbd_unlocked_set_debug_callback (struct nbd_handle *h, nbd_unlocked_clear_debug_callback...

...learn that the machine has + * moved on to DEAD at that time. */ h->in_flight++; if (h->cmds_to_issue != NULL) { @@ -240,7 +246,7 @@ nbd_internal_command_common (struct nbd_handle *h, h->cmds_to_issue = h->cmds_to_issue_tail = cmd; if (nbd_internal_is_state_ready (get_next_state (h)) && nbd_internal_run (h, cmd_issue) == -1) - return -1; + nbd_internal_debug (h, "command queued, ignoring state machine failure"); } return cmd->cookie; -- 2.20.1

These patches aren't related to each other, but both are quite simple. The second one requires particular attention - it's my experience that printing out the state transitions in debug mode has never helped me to diagnose a bug, but it has made the debug logs huge and hard to follow. However that might just be me! Has it helped anyone else? Also I'm open to the concept of debug