search for: nbd_internal_is_state_process

...oll (h, -1) == -1) return -1; } diff --git a/lib/disconnect.c b/lib/disconnect.c index 355a1c9..423edaf 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -29,14 +29,14 @@ int nbd_unlocked_shutdown (struct nbd_handle *h) { - if (nbd_internal_is_state_ready (h->state) || - nbd_internal_is_state_processing (h->state)) { + if (nbd_internal_is_state_ready (get_state (h)) || + nbd_internal_is_state_processing (get_state (h))) { if (nbd_unlocked_aio_disconnect (h, 0) == -1) return -1; } - while (!nbd_internal_is_state_closed (h->state) && - !nbd_internal_i...

...(h)" - | Dead -> "nbd_aio_is_dead (h)" + | Created -> "nbd_internal_is_state_created (state)" + | Connecting -> "nbd_internal_is_state_connecting (state)" + | Connected -> "nbd_internal_is_state_ready (state) || nbd_internal_is_state_processing (state)" + | Closed -> "nbd_internal_is_state_closed (state)" + | Dead -> "nbd_internal_is_state_dead (state)" ) permitted_states in pr " if (!(%s)) {\n" (String.concat " ||\n " tests); - pr "...

...quot;; + let tests = + List.map ( + function + | Created -> "nbd_internal_is_state_created (state)" + | Connecting -> "nbd_internal_is_state_connecting (state)" + | Connected -> "nbd_internal_is_state_ready (state) || nbd_internal_is_state_processing (state)" + | Closed -> "nbd_internal_is_state_closed (state)" + | Dead -> "nbd_internal_is_state_dead (state)" + ) permitted_states in + pr " if (!(%s)) {\n" (String.concat " ||\n " tests); + pr "...

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

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

...l (h, -1) == -1) return -1; } diff --git a/lib/disconnect.c b/lib/disconnect.c index 423edaf..95e9a37 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -29,14 +29,14 @@ int nbd_unlocked_shutdown (struct nbd_handle *h) { - if (nbd_internal_is_state_ready (get_state (h)) || - nbd_internal_is_state_processing (get_state (h))) { + if (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) return -1; } - while (!nbd_internal_is_state_closed (get_state (h)) && - !...

...l (h, -1) == -1) return -1; } diff --git a/lib/disconnect.c b/lib/disconnect.c index 423edaf..95e9a37 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -29,14 +29,14 @@ int nbd_unlocked_shutdown (struct nbd_handle *h) { - if (nbd_internal_is_state_ready (get_state (h)) || - nbd_internal_is_state_processing (get_state (h))) { + if (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) return -1; } - while (!nbd_internal_is_state_closed (get_state (h)) && - !...

...d, 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) return -1; } @@ -57,6 +58,7 @@ nbd_unlock...

...| Dead -> "nbd_aio_is_dead (h)" > + | Created -> "nbd_internal_is_state_created (state)" > + | Connecting -> "nbd_internal_is_state_connecting (state)" > + | Connected -> "nbd_internal_is_state_ready (state) || nbd_internal_is_state_processing (state)" Yes, this fixes the race: this code is executed outside the lock, so it must read h->state exactly once before using it in multiple spots. > +++ b/lib/connect.c > @@ -38,16 +38,16 @@ > static int > error_unless_ready (struct nbd_handle *h) > { > - if (nbd...

...cmd); + assert (h->rlen); + h->reply_cmd->state = get_next_state (h); +} + +/*----- End of prologue. -----*/ /* STATE MACHINE */ { REPLY.START: + /* If rlen is non-zero, we are resuming an earlier reply cycle. */ + if (h->rlen > 0) { + if (h->reply_cmd) { + assert (nbd_internal_is_state_processing (h->reply_cmd->state)); + SET_NEXT_STATE (h->reply_cmd->state); + } + else + SET_NEXT_STATE (%RECV_REPLY); + return 0; + } + /* This state is entered when a read notification is received in the * READY state. Therefore we know the socket is readable here....

..._cmd; + struct command *cmd; 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-&gt...

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 |

...gt;state = get_next_state (h); > +} > + > +/*----- End of prologue. -----*/ > > /* STATE MACHINE */ { > REPLY.START: > + /* If rlen is non-zero, we are resuming an earlier reply cycle. */ > + if (h->rlen > 0) { > + if (h->reply_cmd) { > + assert (nbd_internal_is_state_processing (h->reply_cmd->state)); > + SET_NEXT_STATE (h->reply_cmd->state); This is essentially the "stack of states" idea that I had early on, but with a maximum stack depth of 1. I originally rejected the idea then because it means that the generator would not longer have...

...e_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_processing (get_next_state (h)))) { diff --git a/tests/Makefile.am b/tests/Makefile.am index be7c83c..007c69e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -142,6 +142,7 @@ if HAVE_NBDKIT check_PROGRAMS += \ errors \ server-death \ + shutdown-flags \ get-size \ read-only-flag \ read-...

...(*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_processing (get_next_state (h)))) { > diff --git a/tests/Makefile.am b/tests/Makefile.am > index be7c83c..007c69e 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -142,6 +142,7 @@ if HAVE_NBDKIT > check_PROGRAMS += \ > errors \ > server-death \ > + shutdown-fl...

...nt32_t min, /* is-state.c */ extern bool nbd_internal_is_state_created (enum state state); extern bool nbd_internal_is_state_connecting (enum state state); +extern bool nbd_internal_is_state_negotiating (enum state state); extern bool nbd_internal_is_state_ready (enum state state); extern bool nbd_internal_is_state_processing (enum state state); extern bool nbd_internal_is_state_dead (enum state state); diff --git a/generator/API.ml b/generator/API.ml index 82fdf75..ff7f4c1 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -90,6 +90,7 @@ and flags = { and permitted_state = | Created | Connecting +| Negotiat...

...@ struct socket { const struct socket_ops *ops; }; -struct command_cb { - union { - nbd_extent_callback extent; - nbd_chunk_callback chunk; - } fn; - nbd_completion_callback completion; -}; - struct command { struct command *next; uint16_t flags; @@ -420,6 +418,9 @@ extern bool nbd_internal_is_state_processing (enum state state); extern bool nbd_internal_is_state_dead (enum state state); extern bool nbd_internal_is_state_closed (enum state state); +/* opt.c */ +extern void nbd_internal_free_option (struct nbd_handle *h); + /* protocol.c */ extern int nbd_internal_errno_of_nbd_error (uint32_t erro...

...@ struct socket { const struct socket_ops *ops; }; -struct command_cb { - union { - nbd_extent_callback extent; - nbd_chunk_callback chunk; - } fn; - nbd_completion_callback completion; -}; - struct command { struct command *next; uint16_t flags; @@ -420,6 +418,9 @@ extern bool nbd_internal_is_state_processing (enum state state); extern bool nbd_internal_is_state_dead (enum state state); extern bool nbd_internal_is_state_closed (enum state state); +/* opt.c */ +extern void nbd_internal_free_option (struct nbd_handle *h); + /* protocol.c */ extern int nbd_internal_errno_of_nbd_error (uint32_t erro...

Well, I'm not quite done (I still want to get nbdinfo to work on a single nbd connection for all cases when reading the heads of the file is not required), but I'm happy with patches 1-11, and 12-13 show where I'm headed for getting NBD_OPT_INFO to work. Posting now to see if some of the earlier patches are ready to commit while I continue working on the latter half. Eric Blake (13):

This is a subset of my v2 posting, but limited to just the NBD_OPT_LIST handling. The biggest change since v2 is the addition of added unit testing in all four language bindings (C, python, ocaml, golang). The tests require nbdkit built from git on PATH, and may not be entirely idiomatic, but I at least validated that they catch issues (for example, adding an exit statement near the end of the