On 6/9/23 04:17, Eric Blake wrote:
> 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 <ler...@redhat.com>
> Signed-off-by: Eric Blake <ebl...@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);
Can you perhaps append
... &&
sizeof h->sbuf.simple_reply.cookie ==
sizeof h->sbuf.sr.structured_reply.cookie
(if you agree)?
Also, the commit message and the comment talk about the magic number as
well, not just the cookie, and the static assertion ignores magic.
However, I can see the magic handling changes in the next patch.
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks
Laszlo
> 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);
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
