[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(a)redhat.com&gt; --- 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