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 <ebl...@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 <ler...@redhat.com>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
