Re: [Libguestfs] [libnbd PATCH 2/8] states: Consolidate search for current reply's command

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.

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(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs