Eric Blake
2023-Jun-09 20:39 UTC
[Libguestfs] [libnbd PATCH v4 1/4] states: Document our reliance on type overlaps
[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 at redhat.com> > > Signed-off-by: Eric Blake <eblake at 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
Richard W.M. Jones
2023-Jun-12 18:10 UTC
[Libguestfs] [libnbd PATCH v4 1/4] states: Document our reliance on type overlaps
On Fri, Jun 09, 2023 at 03:39:19PM -0500, Eric Blake wrote:> [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 at redhat.com> > > > Signed-off-by: Eric Blake <eblake at 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);This is a nice idea!> 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?We can always start with the 3 arg version and change it if we need to later. At the moment I can't think of a reason to check that fields in two unrelated types overlap, since you'd presumably always want to use them through an actual union type, but I suppose it could happen.> 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.Additionally, we currently only support GCC and Clang, so anything that works for those only is fine. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Laszlo Ersek
2023-Jun-19 08:32 UTC
[Libguestfs] [libnbd PATCH v4 1/4] states: Document our reliance on type overlaps
On 6/9/23 22:39, Eric Blake wrote:> [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 at redhat.com> >>> Signed-off-by: Eric Blake <eblake at 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?Both 4-arg invocations look fine to me, so I wouldn't push for a 3-arg variant at this time.> 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.I don't recall feeling a need to use a static assertion outside of a function body. My gut feeling is that any given assertion (static or runtime) tends to matter, in the end, for a specific statement; the statement where we "exploit" the particular predicate. If the predicate is supposed to be evaluated at compile time, that may allow for moving the static assertion to a header file, but I'm not convinced it helps readability and/or whether such a code movement is really necessary. For now I feel OK with just sticking our current STATIC_ASSERT(). Laszlo