3:39 p.m.
[Bah - I typed up a longer response, but lost it when accidentally
trying to send through the wrong SMTP server, so now I have to
remember what I had...]
On Fri, Jun 09, 2023 at 02:45:56PM +0200, Laszlo Ersek wrote:
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 <lersek(a)redhat.com>
> Signed-off-by: Eric Blake <eblake(a)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)?
Yes, that makes sense, and I did so for what got pushed as 29342fedb53
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.
I was a bit less concerned about magic (it is easy to see that it is
at offset 0 in both types and could satisfy the common prefix rules,
while seeing cookie's location and a non-common prefix makes the
latter more imporant to assert). But checking two members instead of
one shouldn't hurt, and in fact, once extended types are in (plus
patch 4/4 of this series also adds an anonymous sub-struct in 'union
reply_header' which is also worth validating), it may make sense to do
a followup patch that adds:
#define ASSERT_MEMBER_OVERLAP(TypeA, memberA, TypeB, memberB) \
STATIC_ASSERT (offsetof (TypeA, memberA) == offsetof (TypeB, memberB) && \
sizeof ((TypeA *)NULL)->memberA == sizeof ((TypeB *)NULL)->memberB,
\
member_overlap)
to be used either as:
ASSERT_MEMBER_OVERLAP (struct nbd_simple_reply, cookie,
struct nbd_structured_reply, cookie);
or as
ASSERT_MEMBER_OVERLAP (struct nbd_handle, sbuf.simple_reply.magic,
struct nbd_handle, sbuf.sr.structured_reply.magic);
Would it make sense to have the macro take only three arguments (since
both of those invocations repeat an argument); if so, is it better to
share the common type name, or the common member name?
I also note that our "static-assert.h" file defines STATIC_ASSERT() as
a do/while statement (that is, it MUST appear inside a function body,
so we can't use it easily in .h files); contrast that with C11's
_Static_assert() or qemu's QEMU_BUILD_BUG_ON() that behave more as a
type declaration (and can therefore appear outside of a function body;
C23 will take it one step further by adding static_assert(expr)
alongside static_assert(expr, msg). I consider myself too tainted,
not only by helping with qemu's implementation, but also by reviewing
gnulib's implementation (which uses __VA_ARGS__ to emulate C23
semantics of an optional message), to be able to feel comfortable
trying to improve our static-assert.h for sharing back to nbdkit, but
I don't mind reviewing anyone else's attempts.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org