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
Eric Blake
2023-Jun-12 13:00 UTC
[Libguestfs] [libnbd PATCH v4 4/4] internal: Refactor layout of replies in sbuf
On Thu, Jun 08, 2023 at 09:17:38PM -0500, Eric Blake wrote:> 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. >[...]> > 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(-)Here's what I'm leaning towards squashing in, now that patch 2/4 landed as 2e34ceb8. diff --git a/generator/states-reply.c b/generator/states-reply.c index af5f6135..0d200513 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -19,6 +19,11 @@ #include <assert.h> #include <stddef.h> +#define ASSERT_MEMBER_ALIAS(type, member_a, member_b) \ + STATIC_ASSERT (offsetof (type, member_a) == offsetof (type, member_b) && \ + sizeof ((type *)NULL)->member_a == \ + sizeof ((type *)NULL)->member_b, member_alias) + /* State machine for receiving reply messages from the server. * * Note that we never block while in this sub-group. If there is @@ -70,12 +75,10 @@ REPLY.START: * structs (an intentional design decision in the NBD spec when * structured replies were added). */ - 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_MEMBER_ALIAS (union reply_header, simple.magic, magic); + ASSERT_MEMBER_ALIAS (union reply_header, simple.cookie, cookie); + ASSERT_MEMBER_ALIAS (union reply_header, structured.magic, magic); + ASSERT_MEMBER_ALIAS (union reply_header, structured.cookie, cookie); assert (h->reply_cmd == NULL); assert (h->rlen == 0); -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Laszlo Ersek
2023-Jun-20 13:57 UTC
[Libguestfs] [libnbd PATCH v4 4/4] internal: Refactor layout of replies in sbuf
On 6/9/23 04:17, Eric Blake wrote:> 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, "Acked-by: Laszlo Ersek <lersek at redhat.com>
Possibly Parallel Threads
- [libnbd PATCH 2/8] states: Consolidate search for current reply's command
- Re: [libnbd PATCH] states: Never block state machine inside REPLY
- [libnbd PATCH] states: Validate error message size
- [libnbd PATCH 2/6] generator: Allow DEAD state actions to run
- [libnbd PATCH] states: Never block state machine inside REPLY