[Libguestfs] [libnbd PATCH 2/8] states: Consolidate search for current reply's command
Richard W.M. Jones
rjones at redhat.com
Thu Jun 20 05:46:21 UTC 2019
On Mon, Jun 17, 2019 at 07:07:52PM -0500, Eric Blake wrote:
> No need to have each state recompute which reply is current. This also
> consolidates the logic when a reply has an unexpected handle -
> previously, we failed for structured (even though the length makes it
> easy to recover) and passed for simple (even though there is nothing
> on the wire to state if this response is associated with NBD_CMD_READ
> if we did not negotiate structured replies, which would allow our
> state machine to start parsing arbitrary data as new responses); we're
> better off just always moving to DEAD.
Pretty simple and obvious optimization, ACK.
Rich.
> generator/states-reply-simple.c | 15 ++-----
> generator/states-reply-structured.c | 61 ++++++-----------------------
> generator/states-reply.c | 31 ++++++++++++++-
> lib/internal.h | 2 +
> 4 files changed, 45 insertions(+), 64 deletions(-)
>
> diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
> index 7e5340c..12536e0 100644
> --- a/generator/states-reply-simple.c
> +++ b/generator/states-reply-simple.c
> @@ -20,24 +20,15 @@
>
> /* STATE MACHINE */ {
> REPLY.SIMPLE_REPLY.START:
> - struct command_in_flight *cmd;
> + struct command_in_flight *cmd = h->reply_cmd;
> uint32_t error;
> uint64_t handle;
>
> error = be32toh (h->sbuf.simple_reply.error);
> handle = be64toh (h->sbuf.simple_reply.handle);
>
> - /* Find the command amongst the commands in flight. */
> - for (cmd = h->cmds_in_flight; cmd != NULL; cmd = cmd->next) {
> - if (cmd->handle == handle)
> - break;
> - }
> - if (cmd == NULL) {
> - SET_NEXT_STATE (%.READY);
> - set_error (0, "no matching handle found for server reply, "
> - "this is probably a bug in the server");
> - return -1;
> - }
> + assert (cmd);
> + assert (cmd->handle == handle);
>
> if (cmd->type == NBD_CMD_READ && h->structured_replies) {
> set_error (0, "server sent unexpected simple reply for read");
> diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
> index 2125e41..9bb165b 100644
> --- a/generator/states-reply-structured.c
> +++ b/generator/states-reply-structured.c
> @@ -43,7 +43,7 @@
> return 0;
>
> REPLY.STRUCTURED_REPLY.CHECK:
> - struct command_in_flight *cmd;
> + struct command_in_flight *cmd = h->reply_cmd;
> uint16_t flags, type;
> uint64_t handle;
> uint32_t length;
> @@ -53,20 +53,8 @@
> handle = be64toh (h->sbuf.sr.structured_reply.handle);
> length = be32toh (h->sbuf.sr.structured_reply.length);
>
> - /* Find the command amongst the commands in flight. */
> - for (cmd = h->cmds_in_flight; cmd != NULL; cmd = cmd->next) {
> - if (cmd->handle == handle)
> - break;
> - }
> - if (cmd == NULL) {
> - /* Unlike for simple replies, this is difficult to recover from. We
> - * would need an extra state to read and ignore length bytes. XXX
> - */
> - SET_NEXT_STATE (%.DEAD);
> - set_error (0, "no matching handle found for server reply, "
> - "this is probably a bug in the server");
> - return -1;
> - }
> + assert (cmd);
> + assert (cmd->handle == handle);
>
> /* Reject a server that replies with too much information, but don't
> * reject a single structured reply to NBD_CMD_READ on the largest
> @@ -224,8 +212,7 @@
> return 0;
>
> REPLY.STRUCTURED_REPLY.RECV_ERROR_TAIL:
> - struct command_in_flight *cmd;
> - uint64_t handle;
> + struct command_in_flight *cmd = h->reply_cmd;
> uint32_t error;
> uint64_t offset;
> uint16_t type;
> @@ -233,15 +220,9 @@
> switch (recv_into_rbuf (h)) {
> case -1: SET_NEXT_STATE (%.DEAD); return -1;
> case 0:
> - handle = be64toh (h->sbuf.sr.structured_reply.handle);
> error = be32toh (h->sbuf.sr.payload.error.error.error);
> type = be16toh (h->sbuf.sr.structured_reply.type);
>
> - /* Find the command amongst the commands in flight. */
> - for (cmd = h->cmds_in_flight; cmd != NULL; cmd = cmd->next) {
> - if (cmd->handle == handle)
> - break;
> - }
> assert (cmd); /* guaranteed by CHECK */
>
> /* Sanity check that any error offset is in range */
> @@ -267,23 +248,16 @@
> return 0;
>
> REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA:
> - struct command_in_flight *cmd;
> - uint64_t handle;
> + struct command_in_flight *cmd = h->reply_cmd;
> uint64_t offset;
> uint32_t length;
>
> switch (recv_into_rbuf (h)) {
> case -1: SET_NEXT_STATE (%.DEAD); return -1;
> case 0:
> - handle = be64toh (h->sbuf.sr.structured_reply.handle);
> length = be32toh (h->sbuf.sr.structured_reply.length);
> offset = be64toh (h->sbuf.sr.payload.offset_data.offset);
>
> - /* Find the command amongst the commands in flight. */
> - for (cmd = h->cmds_in_flight; cmd != NULL; cmd = cmd->next) {
> - if (cmd->handle == handle)
> - break;
> - }
> assert (cmd); /* guaranteed by CHECK */
>
> if (cmd->type != NBD_CMD_READ) {
> @@ -336,23 +310,16 @@
> return 0;
>
> REPLY.STRUCTURED_REPLY.RECV_OFFSET_HOLE:
> - struct command_in_flight *cmd;
> - uint64_t handle;
> + struct command_in_flight *cmd = h->reply_cmd;
> uint64_t offset;
> uint32_t length;
>
> switch (recv_into_rbuf (h)) {
> case -1: SET_NEXT_STATE (%.DEAD); return -1;
> case 0:
> - handle = be64toh (h->sbuf.sr.structured_reply.handle);
> offset = be64toh (h->sbuf.sr.payload.offset_hole.offset);
> length = be32toh (h->sbuf.sr.payload.offset_hole.length);
>
> - /* Find the command amongst the commands in flight. */
> - for (cmd = h->cmds_in_flight; cmd != NULL; cmd = cmd->next) {
> - if (cmd->handle == handle)
> - break;
> - }
> assert (cmd); /* guaranteed by CHECK */
>
> if (cmd->type != NBD_CMD_READ) {
> @@ -394,8 +361,7 @@
> return 0;
>
> REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES:
> - struct command_in_flight *cmd;
> - uint64_t handle;
> + struct command_in_flight *cmd = h->reply_cmd;
> uint32_t length;
> size_t i;
> uint32_t context_id;
> @@ -404,16 +370,9 @@
> switch (recv_into_rbuf (h)) {
> case -1: SET_NEXT_STATE (%.DEAD); return -1;
> case 0:
> - handle = be64toh (h->sbuf.sr.structured_reply.handle);
> length = be32toh (h->sbuf.sr.structured_reply.length);
>
> - /* Find the command amongst the commands in flight. */
> - for (cmd = h->cmds_in_flight; cmd != NULL; cmd = cmd->next) {
> - if (cmd->handle == handle)
> - break;
> - }
> - /* guaranteed by CHECK */
> - assert (cmd);
> + assert (cmd); /* guaranteed by CHECK */
> assert (cmd->extent_fn);
> assert (h->bs_entries);
> assert (length >= 12);
> @@ -460,8 +419,10 @@
> flags = be16toh (h->sbuf.sr.structured_reply.flags);
> if (flags & NBD_REPLY_FLAG_DONE)
> SET_NEXT_STATE (%^FINISH_COMMAND);
> - else
> + else {
> + h->reply_cmd = NULL;
> SET_NEXT_STATE (%.READY);
> + }
> return 0;
>
> } /* END STATE MACHINE */
> diff --git a/generator/states-reply.c b/generator/states-reply.c
> index f0ef47c..54f98c5 100644
> --- a/generator/states-reply.c
> +++ b/generator/states-reply.c
> @@ -36,6 +36,8 @@
> h->rbuf = &h->sbuf;
> h->rlen = sizeof h->sbuf.simple_reply;
>
> + assert (h->reply_cmd == NULL);
> +
> r = h->sock->ops->recv (h, h->sock, h->rbuf, h->rlen);
> if (r == -1) {
> /* This should never happen because when we enter this state we
> @@ -69,16 +71,16 @@
> return 0;
>
> REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
> + struct command_in_flight *cmd;
> uint32_t magic;
> + uint64_t handle;
>
> magic = be32toh (h->sbuf.simple_reply.magic);
> if (magic == NBD_SIMPLE_REPLY_MAGIC) {
> SET_NEXT_STATE (%SIMPLE_REPLY.START);
> - return 0;
> }
> else if (magic == NBD_STRUCTURED_REPLY_MAGIC) {
> SET_NEXT_STATE (%STRUCTURED_REPLY.START);
> - return 0;
> }
> else {
> SET_NEXT_STATE (%.DEAD); /* We've probably lost synchronization. */
> @@ -86,6 +88,29 @@
> return -1;
> }
>
> + /* NB: This works for both simple and structured replies because the
> + * handle is stored at the same offset.
> + */
> + handle = be64toh (h->sbuf.simple_reply.handle);
> + /* Find the command amongst the commands in flight. */
> + for (cmd = h->cmds_in_flight; cmd != NULL; cmd = cmd->next) {
> + if (cmd->handle == handle)
> + break;
> + }
> + if (cmd == NULL) {
> + /* An unexpected structured reply could be skipped, since it
> + * includes a length; similarly an unexpected simple reply can be
> + * skipped if we assume it was not a read. However, it's more
> + * likely we've lost synchronization with the server.
> + */
> + SET_NEXT_STATE (%.DEAD);
> + set_error (0, "no matching handle found for server reply, "
> + "this is probably a bug in the server");
> + return -1;
> + }
> + h->reply_cmd = cmd;
> + return 0;
> +
> REPLY.FINISH_COMMAND:
> struct command_in_flight *prev_cmd, *cmd;
> uint64_t handle;
> @@ -102,6 +127,8 @@
> break;
> }
> assert (cmd != NULL);
> + assert (h->reply_cmd == cmd);
> + h->reply_cmd = NULL;
>
> /* Move it to the end of the cmds_done list. */
> if (prev_cmd != NULL)
> diff --git a/lib/internal.h b/lib/internal.h
> index 9404d65..6fde06c 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -189,6 +189,8 @@ struct nbd_handle {
> * acknowledge them.
> */
> struct command_in_flight *cmds_to_issue, *cmds_in_flight, *cmds_done;
> + /* Current command during a REPLY cycle */
> + struct command_in_flight *reply_cmd;
> };
>
> struct meta_context {
> --
> 2.20.1
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
More information about the Libguestfs
mailing list