4:53 p.m.
We had two different flows for a loop ended by error/NBD_REP_ACK:
OPT_GO OPT_SET_META_CONTEXT
- RECV_REPLY - RECV_REPLY
finish read() of reply header finish read() of reply header
set up to read/skip replylen bytes sanity check header
- RECV_REPLY_PAYLOAD switch based on reply type
finish read()/skip of reply payload set up read/skip replylen bytes
- CHECK_REPLY - RECV_REPLY_PAYLOAD
sanity check header finish read() of reply payload
switch based on reply type utilize payload
utilize pre-parsed payload choose next state
choose next state - SKIP_PAYLOAD
finish skip of reply payload
- FINISH
go to next state
I'm finding the OPT_GO flow easier to understand - it has fewer
states, and separates collecting the data from parsing it rather than
splitting the parse across two states. It is also a better idea to
collect reply payload for all reply types, not just our expected one,
if we are to change things to start reporting any server's error
messages appended to NBD_REP_ERR.
The OPT_STARTTLS and OPT_STRUCTURED_REPLY sub-states are also similar
to OPT_GO, except that they currently always skip rather than read()
the payload.
The patch feels rather large; view with 'git diff -b' for a slightly
easier read. But there should be no change in behavior other than a
new debug message when we skip a replied context for being too large.
---
generator/generator | 11 +-
.../states-newstyle-opt-set-meta-context.c | 120 +++++++++---------
2 files changed, 60 insertions(+), 71 deletions(-)
diff --git a/generator/generator b/generator/generator
index deb77f0..e3dd10f 100755
--- a/generator/generator
+++ b/generator/generator
@@ -519,15 +519,8 @@ and newstyle_opt_set_meta_context_state_machine = [
State {
default_state with
- name = "RECV_SKIP_PAYLOAD";
- comment = "Ignore newstyle NBD_OPT_SET_META_CONTEXT option reply payload";
- external_events = [ NotifyRead, "" ];
- };
-
- State {
- default_state with
- name = "FINISH";
- comment = "Finish newstyle NBD_OPT_SET_META_CONTEXT option parsing";
+ name = "CHECK_REPLY";
+ comment = "Check newstyle NBD_OPT_SET_META_CONTEXT option reply";
external_events = [];
};
]
diff --git a/generator/states-newstyle-opt-set-meta-context.c
b/generator/states-newstyle-opt-set-meta-context.c
index 6b5a6d6..feaa295 100644
--- a/generator/states-newstyle-opt-set-meta-context.c
+++ b/generator/states-newstyle-opt-set-meta-context.c
@@ -30,7 +30,7 @@
if (!h->structured_replies ||
h->request_meta_contexts == NULL ||
nbd_internal_string_list_length (h->request_meta_contexts) == 0) {
- SET_NEXT_STATE (%FINISH);
+ SET_NEXT_STATE (%^OPT_GO.START);
return 0;
}
@@ -139,67 +139,68 @@
return 0;
NEWSTYLE.OPT_SET_META_CONTEXT.RECV_REPLY:
- uint64_t magic;
- uint32_t option;
- uint32_t reply;
uint32_t len;
- const uint32_t maxlen = sizeof h->sbuf.or.payload.context;
+ const uint32_t maxpayload = sizeof h->sbuf.or.payload.context;
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return -1;
case 0:
- magic = be64toh (h->sbuf.or.option_reply.magic);
- option = be32toh (h->sbuf.or.option_reply.option);
- reply = be32toh (h->sbuf.or.option_reply.reply);
+ /* Read the following payload if it is short enough to fit in the
+ * static buffer. If it's too long, skip it.
+ */
len = be32toh (h->sbuf.or.option_reply.replylen);
- if (magic != NBD_REP_MAGIC || option != NBD_OPT_SET_META_CONTEXT) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply magic or option");
- return -1;
- }
- switch (reply) {
- case NBD_REP_ACK: /* End of list of replies. */
- if (len != 0) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply length");
- return -1;
- }
- SET_NEXT_STATE (%FINISH);
- break;
- case NBD_REP_META_CONTEXT: /* A context. */
- /* If it's too long, skip over it. */
- if (len > maxlen)
- h->rbuf = NULL;
- else if (len <= sizeof h->sbuf.or.payload.context.context) {
- /* A valid reply has at least one byte in payload.context.str */
- set_error (0, "NBD_REP_META_CONTEXT reply length too small");
- SET_NEXT_STATE (%.DEAD);
- return -1;
- }
- else
- h->rbuf = &h->sbuf.or.payload.context;
- h->rlen = len;
- SET_NEXT_STATE (%RECV_REPLY_PAYLOAD);
- break;
- default:
- /* Anything else is an error, ignore it */
- debug (h, "handshake: unexpected error from "
- "NBD_OPT_SET_META_CONTEXT (%" PRIu32 ")", reply);
+ if (len <= maxpayload)
+ h->rbuf = &h->sbuf.or.payload;
+ else
h->rbuf = NULL;
- h->rlen = len;
- SET_NEXT_STATE (%RECV_SKIP_PAYLOAD);
- }
+ h->rlen = len;
+ SET_NEXT_STATE (%RECV_REPLY_PAYLOAD);
}
return 0;
NEWSTYLE.OPT_SET_META_CONTEXT.RECV_REPLY_PAYLOAD:
+ switch (recv_into_rbuf (h)) {
+ case -1: SET_NEXT_STATE (%.DEAD); return -1;
+ case 0: SET_NEXT_STATE (%CHECK_REPLY);
+ }
+ return 0;
+
+ NEWSTYLE.OPT_SET_META_CONTEXT.CHECK_REPLY:
+ uint64_t magic;
+ uint32_t option;
+ uint32_t reply;
+ uint32_t len;
+ const size_t maxpayload = sizeof h->sbuf.or.payload.context;
struct meta_context *meta_context;
- uint32_t len;
- switch (recv_into_rbuf (h)) {
- case -1: SET_NEXT_STATE (%.DEAD); return -1;
- case 0:
- if (h->rbuf != NULL) {
+ magic = be64toh (h->sbuf.or.option_reply.magic);
+ option = be32toh (h->sbuf.or.option_reply.option);
+ reply = be32toh (h->sbuf.or.option_reply.reply);
+ len = be32toh (h->sbuf.or.option_reply.replylen);
+ if (magic != NBD_REP_MAGIC || option != NBD_OPT_SET_META_CONTEXT) {
+ SET_NEXT_STATE (%.DEAD);
+ set_error (0, "handshake: invalid option reply magic or option");
+ return -1;
+ }
+ switch (reply) {
+ case NBD_REP_ACK: /* End of list of replies. */
+ if (len != 0) {
+ SET_NEXT_STATE (%.DEAD);
+ set_error (0, "handshake: invalid option reply length");
+ return -1;
+ }
+ SET_NEXT_STATE (%^OPT_GO.START);
+ break;
+ case NBD_REP_META_CONTEXT: /* A context. */
+ if (len > maxpayload)
+ debug (h, "skipping too large meta context");
+ else if (len <= sizeof h->sbuf.or.payload.context.context) {
+ /* A valid reply has at least one byte in payload.context.str */
+ set_error (0, "handshake: NBD_REP_META_CONTEXT reply length too small");
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
+ else {
len = be32toh (h->sbuf.or.option_reply.replylen);
meta_context = malloc (sizeof *meta_context);
@@ -225,20 +226,15 @@
h->meta_contexts = meta_context;
}
SET_NEXT_STATE (%PREPARE_FOR_REPLY);
+ break;
+ default:
+ /* Anything else is an error, ignore it */
+ /* XXX display any error message if NBD_REP_ERR_? */
+ debug (h, "handshake: unexpected error from "
+ "NBD_OPT_SET_META_CONTEXT (%" PRIu32 ")", reply);
+ SET_NEXT_STATE (%^OPT_GO.START);
+ break;
}
return 0;
- NEWSTYLE.OPT_SET_META_CONTEXT.RECV_SKIP_PAYLOAD:
- switch (recv_into_rbuf (h)) {
- case -1: SET_NEXT_STATE (%.DEAD); return -1;
- case 0: SET_NEXT_STATE (%FINISH);
- /* XXX: capture instead of skip server's payload to NBD_REP_ERR*? */
- }
- return 0;
-
- NEWSTYLE.OPT_SET_META_CONTEXT.FINISH:
- /* Jump to the next option. */
- SET_NEXT_STATE (%^OPT_GO.START);
- return 0;
-
} /* END STATE MACHINE */
--
2.20.1