Eric Blake
2023-Jun-09 02:17 UTC
[Libguestfs] [libnbd PATCH v4 0/4] Saner reply header layout
This was v3 patch 2/22, reworked to address the confusion about how a
structured reply header is read in two pieces before getting to the
payload portion.
I'm still working on rebasing the rest of my v3 series (patches 1,
3-22) from other comments given, but this seemed independent enough
that it's worth posting now rather than holding it up for the rest of
the series.
Eric Blake (4):
states: Document our reliance on type overlaps
generator: Finish parsing structured header in states-reply.c
generator: Rename states-reply-structured to states-reply-chunk
internal: Refactor layout of replies in sbuf
lib/internal.h | 16 ++-
generator/state_machine.ml | 53 ++++---
generator/states-reply.c | 65 +++++++--
...eply-structured.c => states-reply-chunk.c} | 129 ++++++++----------
generator/states-reply-simple.c | 4 +-
generator/Makefile.am | 2 +-
6 files changed, 143 insertions(+), 126 deletions(-)
rename generator/{states-reply-structured.c => states-reply-chunk.c} (78%)
--
2.40.1
Eric Blake
2023-Jun-09 02:17 UTC
[Libguestfs] [libnbd PATCH v4 1/4] states: Document our reliance on type overlaps
When I added structured replies to the NBD spec, I intentionally chose
a wire layout where the magic number and cookie overlap, even while
the middle member changes from uint32_t error to the pair uint16_t
flags and type. Based only on a strict reading of C rules on
effective types and compatible type prefixes, it's probably
questionable on whether my reliance on type aliasing to reuse cookie
from the same offset of a union, or even the fact that a structured
reply is built by first reading bytes into sbuf.simple_reply then
following up with only bytes into the tail of sbuf.sr.structured_reply
is strictly portable. But since it works in practice, it's worth at
least adding some compile- and run-time assertions that our (ab)use of
aliasing is accessing the bytes we want under the types we expect.
Upcoming patches will restructure part of the sbuf layout to hopefully
be a little easier to tie back to strict C standards.
Suggested-by: Laszlo Ersek <lersek at redhat.com>
Signed-off-by: Eric Blake <eblake at redhat.com>
---
generator/states-reply.c | 17 +++++++++++++----
generator/states-reply-structured.c | 13 +++++++++----
2 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 511e5cb1..2c77658b 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -17,6 +17,7 @@
*/
#include <assert.h>
+#include <stddef.h>
/* State machine for receiving reply messages from the server.
*
@@ -63,9 +64,15 @@ REPLY.START:
ssize_t r;
/* We read all replies initially as if they are simple replies, but
- * check the magic in CHECK_SIMPLE_OR_STRUCTURED_REPLY below.
- * This works because the structured_reply header is larger.
+ * check the magic in CHECK_SIMPLE_OR_STRUCTURED_REPLY below. This
+ * works because the structured_reply header is larger, and because
+ * the last member of a simple reply, cookie, is coincident between
+ * the two structs (an intentional design decision in the NBD spec
+ * when structured replies were added).
*/
+ STATIC_ASSERT (offsetof (struct nbd_handle, sbuf.simple_reply.cookie) =+
offsetof (struct nbd_handle, sbuf.sr.structured_reply.cookie),
+ cookie_aliasing);
assert (h->reply_cmd == NULL);
assert (h->rlen == 0);
@@ -135,7 +142,8 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
}
/* NB: This works for both simple and structured replies because the
- * handle (our cookie) is stored at the same offset.
+ * handle (our cookie) is stored at the same offset. See the
+ * STATIC_ASSERT above in state REPLY.START that confirmed this.
*/
h->chunks_received++;
cookie = be64toh (h->sbuf.simple_reply.cookie);
@@ -155,7 +163,8 @@ REPLY.FINISH_COMMAND:
bool retire;
/* NB: This works for both simple and structured replies because the
- * handle (our cookie) is stored at the same offset.
+ * handle (our cookie) is stored at the same offset. See the
+ * STATIC_ASSERT above in state REPLY.START that confirmed this.
*/
cookie = be64toh (h->sbuf.simple_reply.cookie);
/* Find the command amongst the commands in flight. */
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 5aca7262..205a236d 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -19,6 +19,7 @@
/* State machine for parsing structured replies from the server. */
#include <stdbool.h>
+#include <stddef.h>
#include <stdint.h>
#include <inttypes.h>
@@ -45,11 +46,15 @@ structured_reply_in_bounds (uint64_t offset, uint32_t
length,
STATE_MACHINE {
REPLY.STRUCTURED_REPLY.START:
- /* We've only read the simple_reply. The structured_reply is longer,
- * so read the remaining part.
+ /* We've only read the bytes that fill simple_reply. The
+ * structured_reply is longer, so read the remaining part. We
+ * depend on the memory aliasing in union sbuf to overlay the two
+ * reply types.
*/
- h->rbuf = &h->sbuf;
- h->rbuf = (char *)h->rbuf + sizeof h->sbuf.simple_reply;
+ STATIC_ASSERT (sizeof h->sbuf.simple_reply =+ offsetof
(struct nbd_structured_reply, length),
+ simple_structured_overlap);
+ assert (h->rbuf == (char *)&h->sbuf + sizeof
h->sbuf.simple_reply);
h->rlen = sizeof h->sbuf.sr.structured_reply;
h->rlen -= sizeof h->sbuf.simple_reply;
SET_NEXT_STATE (%RECV_REMAINING);
--
2.40.1
Eric Blake
2023-Jun-09 02:17 UTC
[Libguestfs] [libnbd PATCH v4 2/4] generator: Finish parsing structured header in states-reply.c
Splitting the parse of a 20-byte structured reply header across two
source files (16 bytes overlapping with simple replies in
states-reply.c, the remaining 4 bytes in states-reply-structured.c) is
confusing. The upcoming addition of extended headers will reuse the
payload parsing portion of structured replies, but will parse its
32-byte header completely in states-reply.c. So it is better to have
a single file in charge of parsing all three header sizes (once the
third type is introduced), where we then farm out to the payload
parsers.
To do that, rename REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY to
REPLY.CHECK_REPLY_MAGIC and have it pick up the setup from
REPLY.STRUCTURED_REPLY.START, rename REPLY
REPLY.STRUCTURED_REPLY.RECV_REMAINING to
REPLY.RECV_STRUCTURED_REMAINING across files, and merge
REPLY.STRUCTURED_REPLY.CHECK into what would otherwise be the
now-empty REPLY.STRUCTURED_REPLY.START.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
generator/state_machine.ml | 29 ++++++++-----------
generator/states-reply.c | 43 ++++++++++++++++++++++-------
generator/states-reply-structured.c | 26 -----------------
3 files changed, 44 insertions(+), 54 deletions(-)
diff --git a/generator/state_machine.ml b/generator/state_machine.ml
index 126159b9..b5485aec 100644
--- a/generator/state_machine.ml
+++ b/generator/state_machine.ml
@@ -769,8 +769,15 @@ and
State {
default_state with
- name = "CHECK_SIMPLE_OR_STRUCTURED_REPLY";
- comment = "Check if the reply is a simple or structured reply";
+ name = "CHECK_REPLY_MAGIC";
+ comment = "Check if the reply has expected magic";
+ external_events = [];
+ };
+
+ State {
+ default_state with
+ name = "RECV_STRUCTURED_REMAINING";
+ comment = "Receiving the remaining part of a structured reply
header";
external_events = [];
};
@@ -804,28 +811,14 @@ and
};
]
-(* Receiving a structured reply from the server.
+(* Receiving a structured reply payload from the server.
* Implementation: generator/states-reply-structured.c
*)
and structured_reply_state_machine = [
State {
default_state with
name = "START";
- comment = "Prepare to receive the remaining part of a structured
reply";
- external_events = [];
- };
-
- State {
- default_state with
- name = "RECV_REMAINING";
- comment = "Receiving the remaining part of a structured reply";
- external_events = [];
- };
-
- State {
- default_state with
- name = "CHECK";
- comment = "Parse a structured reply from the server";
+ comment = "Start parsing a structured reply payload from the
server";
external_events = [];
};
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 2c77658b..87f17bae 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -64,11 +64,11 @@ REPLY.START:
ssize_t r;
/* We read all replies initially as if they are simple replies, but
- * check the magic in CHECK_SIMPLE_OR_STRUCTURED_REPLY below. This
- * works because the structured_reply header is larger, and because
- * the last member of a simple reply, cookie, is coincident between
- * the two structs (an intentional design decision in the NBD spec
- * when structured replies were added).
+ * check the magic in CHECK_REPLY_MAGIC below. This works because
+ * the structured_reply header is larger, and because the last
+ * member of a simple reply, cookie, is coincident between the two
+ * structs (an intentional design decision in the NBD spec when
+ * structured replies were added).
*/
STATIC_ASSERT (offsetof (struct nbd_handle, sbuf.simple_reply.cookie) =
offsetof (struct nbd_handle, sbuf.sr.structured_reply.cookie),
@@ -113,11 +113,11 @@ REPLY.RECV_REPLY:
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return 0;
case 1: SET_NEXT_STATE (%.READY); return 0;
- case 0: SET_NEXT_STATE (%CHECK_SIMPLE_OR_STRUCTURED_REPLY);
+ case 0: SET_NEXT_STATE (%CHECK_REPLY_MAGIC);
}
return 0;
- REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
+ REPLY.CHECK_REPLY_MAGIC:
struct command *cmd;
uint32_t magic;
uint64_t cookie;
@@ -127,7 +127,18 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
SET_NEXT_STATE (%SIMPLE_REPLY.START);
}
else if (magic == NBD_STRUCTURED_REPLY_MAGIC) {
- SET_NEXT_STATE (%STRUCTURED_REPLY.START);
+ /* We've only read the bytes that fill simple_reply. The
+ * structured_reply is longer, so prepare to read the remaining
+ * bytes. We depend on the memory aliasing in union sbuf to
+ * overlay the two reply types.
+ */
+ STATIC_ASSERT (sizeof h->sbuf.simple_reply =+ offsetof
(struct nbd_structured_reply, length),
+ simple_structured_overlap);
+ assert (h->rbuf == (char *)&h->sbuf + sizeof
h->sbuf.simple_reply);
+ h->rlen = sizeof h->sbuf.sr.structured_reply;
+ h->rlen -= sizeof h->sbuf.simple_reply;
+ SET_NEXT_STATE (%RECV_STRUCTURED_REMAINING);
}
else {
/* We've probably lost synchronization. */
@@ -141,8 +152,9 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
return 0;
}
- /* NB: This works for both simple and structured replies because the
- * handle (our cookie) is stored at the same offset. See the
+ /* NB: This works for both simple and structured replies, even
+ * though we haven't finished reading the structured header yet,
+ * because the cookie is stored at the same offset. See the
* STATIC_ASSERT above in state REPLY.START that confirmed this.
*/
h->chunks_received++;
@@ -157,6 +169,17 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
h->reply_cmd = cmd;
return 0;
+ REPLY.RECV_STRUCTURED_REMAINING:
+ switch (recv_into_rbuf (h)) {
+ case -1: SET_NEXT_STATE (%.DEAD); return 0;
+ case 1:
+ save_reply_state (h);
+ SET_NEXT_STATE (%.READY);
+ return 0;
+ case 0: SET_NEXT_STATE (%STRUCTURED_REPLY.START);
+ }
+ return 0;
+
REPLY.FINISH_COMMAND:
struct command *prev_cmd, *cmd;
uint64_t cookie;
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 205a236d..3a7a03fd 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -46,32 +46,6 @@ structured_reply_in_bounds (uint64_t offset, uint32_t length,
STATE_MACHINE {
REPLY.STRUCTURED_REPLY.START:
- /* We've only read the bytes that fill simple_reply. The
- * structured_reply is longer, so read the remaining part. We
- * depend on the memory aliasing in union sbuf to overlay the two
- * reply types.
- */
- STATIC_ASSERT (sizeof h->sbuf.simple_reply =- offsetof
(struct nbd_structured_reply, length),
- simple_structured_overlap);
- assert (h->rbuf == (char *)&h->sbuf + sizeof
h->sbuf.simple_reply);
- h->rlen = sizeof h->sbuf.sr.structured_reply;
- h->rlen -= sizeof h->sbuf.simple_reply;
- SET_NEXT_STATE (%RECV_REMAINING);
- return 0;
-
- REPLY.STRUCTURED_REPLY.RECV_REMAINING:
- switch (recv_into_rbuf (h)) {
- case -1: SET_NEXT_STATE (%.DEAD); return 0;
- case 1:
- save_reply_state (h);
- SET_NEXT_STATE (%.READY);
- return 0;
- case 0: SET_NEXT_STATE (%CHECK);
- }
- return 0;
-
- REPLY.STRUCTURED_REPLY.CHECK:
struct command *cmd = h->reply_cmd;
uint16_t flags, type;
uint32_t length;
--
2.40.1
Eric Blake
2023-Jun-09 02:17 UTC
[Libguestfs] [libnbd PATCH v4 3/4] generator: Rename states-reply-structured to states-reply-chunk
Upcoming patches to add extended headers want to share the common
payload parser with structured replies. Renaming the file and the
associated states from "structured" to "chunk" makes it more
obvious
that we will be sharing the code independent from the header style
parsed in the earlier REPLY portion of the state machine.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
generator/state_machine.ml | 26 +++++++++----------
generator/states-reply.c | 2 +-
...eply-structured.c => states-reply-chunk.c} | 22 ++++++++--------
generator/Makefile.am | 2 +-
4 files changed, 26 insertions(+), 26 deletions(-)
rename generator/{states-reply-structured.c => states-reply-chunk.c} (97%)
diff --git a/generator/state_machine.ml b/generator/state_machine.ml
index b5485aec..3a912508 100644
--- a/generator/state_machine.ml
+++ b/generator/state_machine.ml
@@ -782,7 +782,7 @@ and
};
Group ("SIMPLE_REPLY", simple_reply_state_machine);
- Group ("STRUCTURED_REPLY", structured_reply_state_machine);
+ Group ("CHUNK_REPLY", chunk_reply_state_machine);
State {
default_state with
@@ -812,76 +812,76 @@ and
]
(* Receiving a structured reply payload from the server.
- * Implementation: generator/states-reply-structured.c
+ * Implementation: generator/states-reply-chunk.c
*)
-and structured_reply_state_machine = [
+and chunk_reply_state_machine = [
State {
default_state with
name = "START";
- comment = "Start parsing a structured reply payload from the
server";
+ comment = "Start parsing a chunk reply payload from the server";
external_events = [];
};
State {
default_state with
name = "RECV_ERROR";
- comment = "Receive a structured reply error header";
+ comment = "Receive a chunk reply error header";
external_events = []
};
State {
default_state with
name = "RECV_ERROR_MESSAGE";
- comment = "Receive a structured reply error message";
+ comment = "Receive a chunk reply error message";
external_events = [];
};
State {
default_state with
name = "RECV_ERROR_TAIL";
- comment = "Receive a structured reply error tail";
+ comment = "Receive a chunk reply error tail";
external_events = [];
};
State {
default_state with
name = "RECV_OFFSET_DATA";
- comment = "Receive a structured reply offset-data header";
+ comment = "Receive a chunk reply offset-data header";
external_events = [];
};
State {
default_state with
name = "RECV_OFFSET_DATA_DATA";
- comment = "Receive a structured reply offset-data block of data";
+ comment = "Receive a chunk reply offset-data block of data";
external_events = [];
};
State {
default_state with
name = "RECV_OFFSET_HOLE";
- comment = "Receive a structured reply offset-hole header";
+ comment = "Receive a chunk reply offset-hole header";
external_events = [];
};
State {
default_state with
name = "RECV_BS_ENTRIES";
- comment = "Receive a structured reply block-status payload";
+ comment = "Receive a chunk reply block-status payload";
external_events = [];
};
State {
default_state with
name = "RESYNC";
- comment = "Ignore payload of an unexpected structured reply";
+ comment = "Ignore payload of an unexpected chunk reply";
external_events = [];
};
State {
default_state with
name = "FINISH";
- comment = "Finish receiving a structured reply";
+ comment = "Finish receiving a chunk reply";
external_events = [];
};
]
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 87f17bae..bd6336a8 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -176,7 +176,7 @@ REPLY.RECV_STRUCTURED_REMAINING:
save_reply_state (h);
SET_NEXT_STATE (%.READY);
return 0;
- case 0: SET_NEXT_STATE (%STRUCTURED_REPLY.START);
+ case 0: SET_NEXT_STATE (%CHUNK_REPLY.START);
}
return 0;
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-chunk.c
similarity index 97%
rename from generator/states-reply-structured.c
rename to generator/states-reply-chunk.c
index 3a7a03fd..94f3a8ae 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-chunk.c
@@ -16,7 +16,7 @@
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
*/
-/* State machine for parsing structured replies from the server. */
+/* State machine for parsing structured reply chunk payloads from the server.
*/
#include <stdbool.h>
#include <stddef.h>
@@ -45,7 +45,7 @@ structured_reply_in_bounds (uint64_t offset, uint32_t length,
}
STATE_MACHINE {
- REPLY.STRUCTURED_REPLY.START:
+ REPLY.CHUNK_REPLY.START:
struct command *cmd = h->reply_cmd;
uint16_t flags, type;
uint32_t length;
@@ -145,7 +145,7 @@ REPLY.STRUCTURED_REPLY.START:
SET_NEXT_STATE (%RESYNC);
return 0;
- REPLY.STRUCTURED_REPLY.RECV_ERROR:
+ REPLY.CHUNK_REPLY.RECV_ERROR:
struct command *cmd = h->reply_cmd;
uint32_t length, msglen, error;
@@ -184,7 +184,7 @@ REPLY.STRUCTURED_REPLY.RECV_ERROR:
SET_NEXT_STATE (%RESYNC);
return 0;
- REPLY.STRUCTURED_REPLY.RECV_ERROR_MESSAGE:
+ REPLY.CHUNK_REPLY.RECV_ERROR_MESSAGE:
uint32_t length, msglen;
uint16_t type;
@@ -226,7 +226,7 @@ REPLY.STRUCTURED_REPLY.RECV_ERROR_MESSAGE:
}
return 0;
- REPLY.STRUCTURED_REPLY.RECV_ERROR_TAIL:
+ REPLY.CHUNK_REPLY.RECV_ERROR_TAIL:
struct command *cmd = h->reply_cmd;
uint32_t error;
uint16_t type;
@@ -283,7 +283,7 @@ REPLY.STRUCTURED_REPLY.RECV_ERROR_TAIL:
}
return 0;
- REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA:
+ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA:
struct command *cmd = h->reply_cmd;
uint64_t offset;
uint32_t length;
@@ -322,7 +322,7 @@ REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA:
}
return 0;
- REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA:
+ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA_DATA:
struct command *cmd = h->reply_cmd;
uint64_t offset;
uint32_t length;
@@ -353,7 +353,7 @@ REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA:
}
return 0;
- REPLY.STRUCTURED_REPLY.RECV_OFFSET_HOLE:
+ REPLY.CHUNK_REPLY.RECV_OFFSET_HOLE:
struct command *cmd = h->reply_cmd;
uint64_t offset;
uint32_t length;
@@ -403,7 +403,7 @@ REPLY.STRUCTURED_REPLY.RECV_OFFSET_HOLE:
}
return 0;
- REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES:
+ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
struct command *cmd = h->reply_cmd;
uint32_t length;
size_t i;
@@ -457,7 +457,7 @@ REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES:
}
return 0;
- REPLY.STRUCTURED_REPLY.RESYNC:
+ REPLY.CHUNK_REPLY.RESYNC:
struct command *cmd = h->reply_cmd;
uint16_t type;
uint32_t length;
@@ -491,7 +491,7 @@ REPLY.STRUCTURED_REPLY.RESYNC:
}
return 0;
- REPLY.STRUCTURED_REPLY.FINISH:
+ REPLY.CHUNK_REPLY.FINISH:
uint16_t flags;
flags = be16toh (h->sbuf.sr.structured_reply.flags);
diff --git a/generator/Makefile.am b/generator/Makefile.am
index 91dbde5c..39285e82 100644
--- a/generator/Makefile.am
+++ b/generator/Makefile.am
@@ -37,8 +37,8 @@ states_code = \
states-newstyle-opt-structured-reply.c \
states-newstyle.c \
states-oldstyle.c \
+ states-reply-chunk.c \
states-reply-simple.c \
- states-reply-structured.c \
states-reply.c \
$(NULL)
--
2.40.1
Eric Blake
2023-Jun-09 02:17 UTC
[Libguestfs] [libnbd PATCH v4 4/4] internal: Refactor layout of replies in sbuf
In order to more easily add a third reply type with an even larger
header, but where the payload will look the same for both structured
and extended replies, it is nicer if simple and structured replies are
nested inside the same layer of sbuf.reply.hdr. Doing this also lets
us add an alias for accessing the cookie directly without picking
which header type we have on hand.
Visually, this patch changes the layout (on x86_64) from:
offset simple structured
+------------------------------------------------------------+
| union sbuf |
| +---------------------+------------------------------+ |
| | struct simple_reply | struct sr | |
| | +-----------------+ | +--------------------------+ | |
| | | | | | struct structured_reply | | |
| | | | | | +----------------------+ | | |
| 0 | | uint32_t magic | | | | uint32_t magic | | | |
| 4 | | uint32_t error | | | | uint16_t flags | | | |
| 6 | | | | | | uint16_t type | | | |
| 8 | | uint64_t cookie | | | | uint64_t cookie | | | |
| | +-----------------+ | | | | | | |
| 16 | [padding] | | | uint32_t length | | | |
| | | | +----------------------+ | | |
| 20 | | | [padding] | | |
| | | | union payload | | |
| | | | +-----------+----------+ | | |
| 24 | | | | ... | ... | | | |
| | | | +-----------+----------+ | | |
| | | +--------------------------+ | |
| +---------------------+------------------------------+ |
+------------------------------------------------------------+
to:
offset simple structured
+-------------------------------------------------------------+
| union sbuf |
| +-----------------------------------------------------+ |
| | struct reply | |
| | +-------------------------------------------------+ | |
| | | union hdr | | |
| | | +--------------------+------------------------+ | | |
| | | | struct simple | struct structured | | | |
| | | | +----------------+ | +--------------------+ | | | |
| 0 | | | | uint32_t magic | | | uint32_t magic | | | | |
| 4 | | | | uint32_t error | | | uint16_t flags | | | | |
| 6 | | | | | | | uint16_t type | | | | |
| 8 | | | | uint64_t cookie| | | uint64_t cookie | | | | |
| | | | +----------------+ | | | | | | |
| 16 | | | [padding] | | uint32_t length | | | | |
| | | | | +--------------------+ | | | |
| 20 | | | | [padding] | | | |
| | | +--------------------+------------------------+ | | |
| | | union payload | | |
| | | +--------------------+------------------------+ | | |
| 24 | | | ... | ... | | | |
| | | +--------------------+------------------------+ | | |
| | +-------------------------------------------------+ | |
| +-----------------------------------------------------+ |
+-------------------------------------------------------------+
Although we do not use union payload with simple replies, a later
patch does use payload for two out of the three possible headers, and
it does not hurt for sbuf to call out storage for more than we use on
a particular state machine sequence. The commit is largely
mechanical, and there should be no semantic change.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
lib/internal.h | 16 +++++--
generator/states-reply.c | 35 +++++++-------
generator/states-reply-chunk.c | 84 ++++++++++++++++-----------------
generator/states-reply-simple.c | 4 +-
4 files changed, 76 insertions(+), 63 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index a8c49e16..f9b10774 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -237,9 +237,19 @@ struct nbd_handle {
} payload;
} or;
struct nbd_export_name_option_reply export_name_reply;
- struct nbd_simple_reply simple_reply;
struct {
- struct nbd_structured_reply structured_reply;
+ union reply_header {
+ struct nbd_simple_reply simple;
+ struct nbd_structured_reply structured;
+ /* The wire formats share magic and cookie at the same offsets;
+ * provide aliases for one less level of typing.
+ */
+ struct {
+ uint32_t magic;
+ uint32_t pad_;
+ uint64_t cookie;
+ };
+ } hdr;
union {
uint64_t align_; /* Start sr.payload on an 8-byte alignment */
struct nbd_structured_reply_offset_data offset_data;
@@ -250,7 +260,7 @@ struct nbd_handle {
uint64_t offset; /* Only used for NBD_REPLY_TYPE_ERROR_OFFSET */
} NBD_ATTRIBUTE_PACKED error;
} payload;
- } sr;
+ } reply;
uint16_t gflags;
uint32_t cflags;
uint32_t len;
diff --git a/generator/states-reply.c b/generator/states-reply.c
index bd6336a8..af5f6135 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -70,14 +70,17 @@ REPLY.START:
* structs (an intentional design decision in the NBD spec when
* structured replies were added).
*/
- STATIC_ASSERT (offsetof (struct nbd_handle, sbuf.simple_reply.cookie) =-
offsetof (struct nbd_handle, sbuf.sr.structured_reply.cookie),
- cookie_aliasing);
+ STATIC_ASSERT (offsetof (union reply_header, simple.cookie) =+
offsetof (union reply_header, cookie),
+ _cookie_aliasing_simple);
+ STATIC_ASSERT (offsetof (union reply_header, structured.cookie) =+
offsetof (union reply_header, cookie),
+ _cookie_aliasing_structured);
assert (h->reply_cmd == NULL);
assert (h->rlen == 0);
- h->rbuf = &h->sbuf;
- h->rlen = sizeof h->sbuf.simple_reply;
+ h->rbuf = &h->sbuf.reply.hdr;
+ h->rlen = sizeof h->sbuf.reply.hdr.simple;
r = h->sock->ops->recv (h, h->sock, h->rbuf, h->rlen);
if (r == -1) {
@@ -122,22 +125,22 @@ REPLY.CHECK_REPLY_MAGIC:
uint32_t magic;
uint64_t cookie;
- magic = be32toh (h->sbuf.simple_reply.magic);
+ magic = be32toh (h->sbuf.reply.hdr.magic);
if (magic == NBD_SIMPLE_REPLY_MAGIC) {
SET_NEXT_STATE (%SIMPLE_REPLY.START);
}
else if (magic == NBD_STRUCTURED_REPLY_MAGIC) {
- /* We've only read the bytes that fill simple_reply. The
- * structured_reply is longer, so prepare to read the remaining
+ /* We've only read the bytes that fill hdr.simple. But
+ * hdr.structured is longer, so prepare to read the remaining
* bytes. We depend on the memory aliasing in union sbuf to
* overlay the two reply types.
*/
- STATIC_ASSERT (sizeof h->sbuf.simple_reply =+ STATIC_ASSERT (sizeof
h->sbuf.reply.hdr.simple = offsetof (struct
nbd_structured_reply, length),
simple_structured_overlap);
- assert (h->rbuf == (char *)&h->sbuf + sizeof
h->sbuf.simple_reply);
- h->rlen = sizeof h->sbuf.sr.structured_reply;
- h->rlen -= sizeof h->sbuf.simple_reply;
+ assert (h->rbuf == (char *)&h->sbuf + sizeof
h->sbuf.reply.hdr.simple);
+ h->rlen = sizeof h->sbuf.reply.hdr.structured;
+ h->rlen -= sizeof h->sbuf.reply.hdr.simple;
SET_NEXT_STATE (%RECV_STRUCTURED_REMAINING);
}
else {
@@ -145,8 +148,8 @@ REPLY.CHECK_REPLY_MAGIC:
SET_NEXT_STATE (%.DEAD);
set_error (0, "invalid reply magic 0x%" PRIx32, magic);
#if 0 /* uncomment to see desynchronized data */
- nbd_internal_hexdump (&h->sbuf.simple_reply,
- sizeof (h->sbuf.simple_reply),
+ nbd_internal_hexdump (&h->sbuf.reply.hdr.simple,
+ sizeof (h->sbuf.reply.hdr.simple),
stderr);
#endif
return 0;
@@ -158,7 +161,7 @@ REPLY.CHECK_REPLY_MAGIC:
* STATIC_ASSERT above in state REPLY.START that confirmed this.
*/
h->chunks_received++;
- cookie = be64toh (h->sbuf.simple_reply.cookie);
+ cookie = be64toh (h->sbuf.reply.hdr.cookie);
/* Find the command amongst the commands in flight. If the server sends
* a reply for an unknown cookie, FINISH will diagnose that later.
*/
@@ -189,7 +192,7 @@ REPLY.FINISH_COMMAND:
* handle (our cookie) is stored at the same offset. See the
* STATIC_ASSERT above in state REPLY.START that confirmed this.
*/
- cookie = be64toh (h->sbuf.simple_reply.cookie);
+ cookie = be64toh (h->sbuf.reply.hdr.cookie);
/* Find the command amongst the commands in flight. */
for (cmd = h->cmds_in_flight, prev_cmd = NULL;
cmd != NULL;
diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
index 94f3a8ae..c42741fb 100644
--- a/generator/states-reply-chunk.c
+++ b/generator/states-reply-chunk.c
@@ -50,9 +50,9 @@ REPLY.CHUNK_REPLY.START:
uint16_t flags, type;
uint32_t length;
- flags = be16toh (h->sbuf.sr.structured_reply.flags);
- type = be16toh (h->sbuf.sr.structured_reply.type);
- length = be32toh (h->sbuf.sr.structured_reply.length);
+ flags = be16toh (h->sbuf.reply.hdr.structured.flags);
+ type = be16toh (h->sbuf.reply.hdr.structured.type);
+ length = be32toh (h->sbuf.reply.hdr.structured.length);
/* Reject a server that replies with too much information, but don't
* reject a single structured reply to NBD_CMD_READ on the largest
@@ -61,7 +61,7 @@ REPLY.CHUNK_REPLY.START:
* oversized reply is going to take long enough to resync that it is
* not worth keeping the connection alive.
*/
- if (length > MAX_REQUEST_SIZE + sizeof h->sbuf.sr.payload.offset_data)
{
+ if (length > MAX_REQUEST_SIZE + sizeof
h->sbuf.reply.payload.offset_data) {
set_error (0, "invalid server reply length %" PRIu32, length);
SET_NEXT_STATE (%.DEAD);
return 0;
@@ -84,19 +84,19 @@ REPLY.CHUNK_REPLY.START:
* them as an extension, so we use < instead of <=.
*/
if (cmd->type != NBD_CMD_READ ||
- length < sizeof h->sbuf.sr.payload.offset_data)
+ length < sizeof h->sbuf.reply.payload.offset_data)
goto resync;
- h->rbuf = &h->sbuf.sr.payload.offset_data;
- h->rlen = sizeof h->sbuf.sr.payload.offset_data;
+ h->rbuf = &h->sbuf.reply.payload.offset_data;
+ h->rlen = sizeof h->sbuf.reply.payload.offset_data;
SET_NEXT_STATE (%RECV_OFFSET_DATA);
break;
case NBD_REPLY_TYPE_OFFSET_HOLE:
if (cmd->type != NBD_CMD_READ ||
- length != sizeof h->sbuf.sr.payload.offset_hole)
+ length != sizeof h->sbuf.reply.payload.offset_hole)
goto resync;
- h->rbuf = &h->sbuf.sr.payload.offset_hole;
- h->rlen = sizeof h->sbuf.sr.payload.offset_hole;
+ h->rbuf = &h->sbuf.reply.payload.offset_hole;
+ h->rlen = sizeof h->sbuf.reply.payload.offset_hole;
SET_NEXT_STATE (%RECV_OFFSET_HOLE);
break;
@@ -127,10 +127,10 @@ REPLY.CHUNK_REPLY.START:
* compliant, will favor the wire error over EPROTO during more
* length checks in RECV_ERROR_MESSAGE and RECV_ERROR_TAIL.
*/
- if (length < sizeof h->sbuf.sr.payload.error.error.error)
+ if (length < sizeof h->sbuf.reply.payload.error.error.error)
goto resync;
- h->rbuf = &h->sbuf.sr.payload.error.error;
- h->rlen = MIN (length, sizeof h->sbuf.sr.payload.error.error);
+ h->rbuf = &h->sbuf.reply.payload.error.error;
+ h->rlen = MIN (length, sizeof h->sbuf.reply.payload.error.error);
SET_NEXT_STATE (%RECV_ERROR);
}
else
@@ -156,19 +156,19 @@ REPLY.CHUNK_REPLY.RECV_ERROR:
SET_NEXT_STATE (%.READY);
return 0;
case 0:
- length = be32toh (h->sbuf.sr.structured_reply.length);
- assert (length >= sizeof h->sbuf.sr.payload.error.error.error);
+ length = be32toh (h->sbuf.reply.hdr.structured.length);
+ assert (length >= sizeof h->sbuf.reply.payload.error.error.error);
assert (cmd);
- if (length < sizeof h->sbuf.sr.payload.error.error)
+ if (length < sizeof h->sbuf.reply.payload.error.error)
goto resync;
- msglen = be16toh (h->sbuf.sr.payload.error.error.len);
- if (msglen > length - sizeof h->sbuf.sr.payload.error.error ||
- msglen > sizeof h->sbuf.sr.payload.error.msg)
+ msglen = be16toh (h->sbuf.reply.payload.error.error.len);
+ if (msglen > length - sizeof h->sbuf.reply.payload.error.error ||
+ msglen > sizeof h->sbuf.reply.payload.error.msg)
goto resync;
- h->rbuf = h->sbuf.sr.payload.error.msg;
+ h->rbuf = h->sbuf.reply.payload.error.msg;
h->rlen = msglen;
SET_NEXT_STATE (%RECV_ERROR_MESSAGE);
}
@@ -176,11 +176,11 @@ REPLY.CHUNK_REPLY.RECV_ERROR:
resync:
/* Favor the error packet's errno over RESYNC's EPROTO. */
- error = be32toh (h->sbuf.sr.payload.error.error.error);
+ error = be32toh (h->sbuf.reply.payload.error.error.error);
if (cmd->error == 0)
cmd->error = nbd_internal_errno_of_nbd_error (error);
h->rbuf = NULL;
- h->rlen = length - MIN (length, sizeof h->sbuf.sr.payload.error.error);
+ h->rlen = length - MIN (length, sizeof
h->sbuf.reply.payload.error.error);
SET_NEXT_STATE (%RESYNC);
return 0;
@@ -195,15 +195,15 @@ REPLY.CHUNK_REPLY.RECV_ERROR_MESSAGE:
SET_NEXT_STATE (%.READY);
return 0;
case 0:
- length = be32toh (h->sbuf.sr.structured_reply.length);
- msglen = be16toh (h->sbuf.sr.payload.error.error.len);
- type = be16toh (h->sbuf.sr.structured_reply.type);
+ length = be32toh (h->sbuf.reply.hdr.structured.length);
+ msglen = be16toh (h->sbuf.reply.payload.error.error.len);
+ type = be16toh (h->sbuf.reply.hdr.structured.type);
- length -= sizeof h->sbuf.sr.payload.error.error + msglen;
+ length -= sizeof h->sbuf.reply.payload.error.error + msglen;
if (msglen)
debug (h, "structured error server message: %.*s", (int)msglen,
- h->sbuf.sr.payload.error.msg);
+ h->sbuf.reply.payload.error.msg);
/* Special case two specific errors; silently ignore tail for all others */
h->rbuf = NULL;
@@ -215,11 +215,11 @@ REPLY.CHUNK_REPLY.RECV_ERROR_MESSAGE:
"the server may have a bug");
break;
case NBD_REPLY_TYPE_ERROR_OFFSET:
- if (length != sizeof h->sbuf.sr.payload.error.offset)
+ if (length != sizeof h->sbuf.reply.payload.error.offset)
debug (h, "unable to safely extract error offset, "
"the server may have a bug");
else
- h->rbuf = &h->sbuf.sr.payload.error.offset;
+ h->rbuf = &h->sbuf.reply.payload.error.offset;
break;
}
SET_NEXT_STATE (%RECV_ERROR_TAIL);
@@ -238,8 +238,8 @@ REPLY.CHUNK_REPLY.RECV_ERROR_TAIL:
SET_NEXT_STATE (%.READY);
return 0;
case 0:
- error = be32toh (h->sbuf.sr.payload.error.error.error);
- type = be16toh (h->sbuf.sr.structured_reply.type);
+ error = be32toh (h->sbuf.reply.payload.error.error.error);
+ type = be16toh (h->sbuf.reply.hdr.structured.type);
assert (cmd); /* guaranteed by CHECK */
@@ -254,7 +254,7 @@ REPLY.CHUNK_REPLY.RECV_ERROR_TAIL:
* user callback if present. Ignore the offset if it was bogus.
*/
if (type == NBD_REPLY_TYPE_ERROR_OFFSET && h->rbuf) {
- uint64_t offset = be64toh (h->sbuf.sr.payload.error.offset);
+ uint64_t offset = be64toh (h->sbuf.reply.payload.error.offset);
if (structured_reply_in_bounds (offset, 0, cmd) &&
cmd->type == NBD_CMD_READ &&
CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
@@ -295,8 +295,8 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA:
SET_NEXT_STATE (%.READY);
return 0;
case 0:
- length = be32toh (h->sbuf.sr.structured_reply.length);
- offset = be64toh (h->sbuf.sr.payload.offset_data.offset);
+ length = be32toh (h->sbuf.reply.hdr.structured.length);
+ offset = be64toh (h->sbuf.reply.payload.offset_data.offset);
assert (cmd); /* guaranteed by CHECK */
@@ -334,8 +334,8 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA_DATA:
SET_NEXT_STATE (%.READY);
return 0;
case 0:
- length = be32toh (h->sbuf.sr.structured_reply.length);
- offset = be64toh (h->sbuf.sr.payload.offset_data.offset);
+ length = be32toh (h->sbuf.reply.hdr.structured.length);
+ offset = be64toh (h->sbuf.reply.payload.offset_data.offset);
assert (cmd); /* guaranteed by CHECK */
if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
@@ -365,8 +365,8 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_HOLE:
SET_NEXT_STATE (%.READY);
return 0;
case 0:
- offset = be64toh (h->sbuf.sr.payload.offset_hole.offset);
- length = be32toh (h->sbuf.sr.payload.offset_hole.length);
+ offset = be64toh (h->sbuf.reply.payload.offset_hole.offset);
+ length = be32toh (h->sbuf.reply.payload.offset_hole.length);
assert (cmd); /* guaranteed by CHECK */
@@ -416,7 +416,7 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
SET_NEXT_STATE (%.READY);
return 0;
case 0:
- length = be32toh (h->sbuf.sr.structured_reply.length);
+ length = be32toh (h->sbuf.reply.hdr.structured.length);
assert (cmd); /* guaranteed by CHECK */
assert (cmd->type == NBD_CMD_BLOCK_STATUS);
@@ -479,8 +479,8 @@ REPLY.CHUNK_REPLY.RESYNC:
SET_NEXT_STATE (%^FINISH_COMMAND);
return 0;
}
- type = be16toh (h->sbuf.sr.structured_reply.type);
- length = be32toh (h->sbuf.sr.structured_reply.length);
+ type = be16toh (h->sbuf.reply.hdr.structured.type);
+ length = be32toh (h->sbuf.reply.hdr.structured.length);
debug (h, "unexpected reply type %u or payload length %" PRIu32
" for cookie %" PRIu64 " and command %" PRIu32
", this is probably a server bug",
@@ -494,7 +494,7 @@ REPLY.CHUNK_REPLY.RESYNC:
REPLY.CHUNK_REPLY.FINISH:
uint16_t flags;
- flags = be16toh (h->sbuf.sr.structured_reply.flags);
+ flags = be16toh (h->sbuf.reply.hdr.structured.flags);
if (flags & NBD_REPLY_FLAG_DONE) {
SET_NEXT_STATE (%^FINISH_COMMAND);
}
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 19be5418..898bb84e 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -23,7 +23,7 @@ REPLY.SIMPLE_REPLY.START:
struct command *cmd = h->reply_cmd;
uint32_t error;
- error = be32toh (h->sbuf.simple_reply.error);
+ error = be32toh (h->sbuf.reply.hdr.simple.error);
if (cmd == NULL) {
/* Unexpected reply. If error was set or we have structured
@@ -39,7 +39,7 @@ REPLY.SIMPLE_REPLY.START:
if (error || h->structured_replies)
SET_NEXT_STATE (%^FINISH_COMMAND);
else {
- uint64_t cookie = be64toh (h->sbuf.simple_reply.cookie);
+ uint64_t cookie = be64toh (h->sbuf.reply.hdr.simple.cookie);
SET_NEXT_STATE (%.DEAD);
set_error (EPROTO,
"no matching cookie %" PRIu64 " found for
server reply, "
--
2.40.1