9:17 p.m.
This was v3 patch 2/22, reworked to address the confusion about how a
structured reply header is read in two pieces before getting to the
payload portion.
I'm still working on rebasing the rest of my v3 series (patches 1,
3-22) from other comments given, but this seemed independent enough
that it's worth posting now rather than holding it up for the rest of
the series.
Eric Blake (4):
states: Document our reliance on type overlaps
generator: Finish parsing structured header in states-reply.c
generator: Rename states-reply-structured to states-reply-chunk
internal: Refactor layout of replies in sbuf
lib/internal.h | 16 ++-
generator/state_machine.ml | 53 ++++---
generator/states-reply.c | 65 +++++++--
...eply-structured.c => states-reply-chunk.c} | 129 ++++++++----------
generator/states-reply-simple.c | 4 +-
generator/Makefile.am | 2 +-
6 files changed, 143 insertions(+), 126 deletions(-)
rename generator/{states-reply-structured.c => states-reply-chunk.c} (78%)
--
2.40.1
9:17 p.m.
New subject: [libnbd PATCH v4 1/4] states: Document our reliance on type overlaps
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);
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);
--
2.40.1
7:45 a.m.
New subject: [libnbd PATCH v4 1/4] states: Document our reliance on type overlaps
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)?
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 <lersek(a)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);
3:39 p.m.
New subject: [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(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
1:10 p.m.
New subject: [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(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);
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
6:49 a.m.
New subject: [libnbd PATCH v4 1/4] states: Document our reliance on type overlaps
On 6/12/23 20:10, Richard W.M. Jones wrote:
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(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);
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.
That's a good point!
> 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.
Sure, but where is it beneficial to put a static assertion outside of a
function body?
I can imagine using sizeofs and offsetofs in declarations, and we might
want to assert various overlaps (or other predicates) right there
(before those dependent declarations). I can see this being useful in
theory, but I don't see a practical need for it in libnbd / nbdkit at
the moment. Do it when it becomes a plus?
Laszlo
3:32 a.m.
New subject: [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(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?
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
9:17 p.m.
New subject: [libnbd PATCH v4 2/4] generator: Finish parsing structured header in states-reply.c
Splitting the parse of a 20-byte structured reply header across two
source files (16 bytes overlapping with simple replies in
states-reply.c, the remaining 4 bytes in states-reply-structured.c) is
confusing. The upcoming addition of extended headers will reuse the
payload parsing portion of structured replies, but will parse its
32-byte header completely in states-reply.c. So it is better to have
a single file in charge of parsing all three header sizes (once the
third type is introduced), where we then farm out to the payload
parsers.
To do that, rename REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY to
REPLY.CHECK_REPLY_MAGIC and have it pick up the setup from
REPLY.STRUCTURED_REPLY.START, rename REPLY
REPLY.STRUCTURED_REPLY.RECV_REMAINING to
REPLY.RECV_STRUCTURED_REMAINING across files, and merge
REPLY.STRUCTURED_REPLY.CHECK into what would otherwise be the
now-empty REPLY.STRUCTURED_REPLY.START.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
generator/state_machine.ml | 29 ++++++++-----------
generator/states-reply.c | 43 ++++++++++++++++++++++-------
generator/states-reply-structured.c | 26 -----------------
3 files changed, 44 insertions(+), 54 deletions(-)
diff --git a/generator/state_machine.ml b/generator/state_machine.ml
index 126159b9..b5485aec 100644
--- a/generator/state_machine.ml
+++ b/generator/state_machine.ml
@@ -769,8 +769,15 @@ and
State {
default_state with
- name = "CHECK_SIMPLE_OR_STRUCTURED_REPLY";
- comment = "Check if the reply is a simple or structured reply";
+ name = "CHECK_REPLY_MAGIC";
+ comment = "Check if the reply has expected magic";
+ external_events = [];
+ };
+
+ State {
+ default_state with
+ name = "RECV_STRUCTURED_REMAINING";
+ comment = "Receiving the remaining part of a structured reply header";
external_events = [];
};
@@ -804,28 +811,14 @@ and
};
]
-(* Receiving a structured reply from the server.
+(* Receiving a structured reply payload from the server.
* Implementation: generator/states-reply-structured.c
*)
and structured_reply_state_machine = [
State {
default_state with
name = "START";
- comment = "Prepare to receive the remaining part of a structured reply";
- external_events = [];
- };
-
- State {
- default_state with
- name = "RECV_REMAINING";
- comment = "Receiving the remaining part of a structured reply";
- external_events = [];
- };
-
- State {
- default_state with
- name = "CHECK";
- comment = "Parse a structured reply from the server";
+ comment = "Start parsing a structured reply payload from the server";
external_events = [];
};
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 2c77658b..87f17bae 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -64,11 +64,11 @@ 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, 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).
+ * check the magic in CHECK_REPLY_MAGIC 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),
@@ -113,11 +113,11 @@ REPLY.RECV_REPLY:
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return 0;
case 1: SET_NEXT_STATE (%.READY); return 0;
- case 0: SET_NEXT_STATE (%CHECK_SIMPLE_OR_STRUCTURED_REPLY);
+ case 0: SET_NEXT_STATE (%CHECK_REPLY_MAGIC);
}
return 0;
- REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
+ REPLY.CHECK_REPLY_MAGIC:
struct command *cmd;
uint32_t magic;
uint64_t cookie;
@@ -127,7 +127,18 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
SET_NEXT_STATE (%SIMPLE_REPLY.START);
}
else if (magic == NBD_STRUCTURED_REPLY_MAGIC) {
- SET_NEXT_STATE (%STRUCTURED_REPLY.START);
+ /* We've only read the bytes that fill simple_reply. The
+ * structured_reply 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 ==
+ 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_STRUCTURED_REMAINING);
}
else {
/* We've probably lost synchronization. */
@@ -141,8 +152,9 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
return 0;
}
- /* NB: This works for both simple and structured replies because the
- * handle (our cookie) is stored at the same offset. See the
+ /* NB: This works for both simple and structured replies, even
+ * though we haven't finished reading the structured header yet,
+ * because the cookie is stored at the same offset. See the
* STATIC_ASSERT above in state REPLY.START that confirmed this.
*/
h->chunks_received++;
@@ -157,6 +169,17 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
h->reply_cmd = cmd;
return 0;
+ REPLY.RECV_STRUCTURED_REMAINING:
+ switch (recv_into_rbuf (h)) {
+ case -1: SET_NEXT_STATE (%.DEAD); return 0;
+ case 1:
+ save_reply_state (h);
+ SET_NEXT_STATE (%.READY);
+ return 0;
+ case 0: SET_NEXT_STATE (%STRUCTURED_REPLY.START);
+ }
+ return 0;
+
REPLY.FINISH_COMMAND:
struct command *prev_cmd, *cmd;
uint64_t cookie;
diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
index 205a236d..3a7a03fd 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -46,32 +46,6 @@ structured_reply_in_bounds (uint64_t offset, uint32_t length,
STATE_MACHINE {
REPLY.STRUCTURED_REPLY.START:
- /* 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.
- */
- 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);
- return 0;
-
- REPLY.STRUCTURED_REPLY.RECV_REMAINING:
- switch (recv_into_rbuf (h)) {
- case -1: SET_NEXT_STATE (%.DEAD); return 0;
- case 1:
- save_reply_state (h);
- SET_NEXT_STATE (%.READY);
- return 0;
- case 0: SET_NEXT_STATE (%CHECK);
- }
- return 0;
-
- REPLY.STRUCTURED_REPLY.CHECK:
struct command *cmd = h->reply_cmd;
uint16_t flags, type;
uint32_t length;
--
2.40.1
10:22 a.m.
New subject: [libnbd PATCH v4 2/4] generator: Finish parsing structured header in states-reply.c
On 6/9/23 04:17, Eric Blake wrote:
Splitting the parse of a 20-byte structured reply header across two
source files (16 bytes overlapping with simple replies in
states-reply.c, the remaining 4 bytes in states-reply-structured.c) is
confusing. The upcoming addition of extended headers will reuse the
payload parsing portion of structured replies, but will parse its
32-byte header completely in states-reply.c. So it is better to have
a single file in charge of parsing all three header sizes (once the
third type is introduced), where we then farm out to the payload
parsers.
"Farm out" is a transitive phrasal verb, but we don't have an object for
it. "Fan out" perhaps? (Or add an object.)
To do that, rename REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY to
REPLY.CHECK_REPLY_MAGIC and have it pick up the setup from
REPLY.STRUCTURED_REPLY.START, rename REPLY
The last REPLY is superfluous
REPLY.STRUCTURED_REPLY.RECV_REMAINING to
REPLY.RECV_STRUCTURED_REMAINING across files, and merge
REPLY.STRUCTURED_REPLY.CHECK into what would otherwise be the
now-empty REPLY.STRUCTURED_REPLY.START.
A different way to describe the same restructuring (I needed this
wording for explaining the code movement to myself):
- We have (prepare, receive, parse) triplets.
- Before the patch, our starter triplet is (REPLY.START,
REPLY.RECV_REPLY, REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY). These are all
in "states-reply.c". In the last component of that triplet (the
"parse"
component), we branch either to SIMPLE_REPLY.START
(states-reply-simple.c) for actual parsing, or we start another triplet,
for the remaining bytes of a structured reply:
(REPLY.STRUCTURED_REPLY.START, REPLY.STRUCTURED_REPLY.RECV_REMAINING,
REPLY.STRUCTURED_REPLY.CHECK); these are all in
- The patch effectively redistributes the second triplet. The prepare
stage REPLY.STRUCTURED_REPLY.START is merged into the last (parse) stage
of the first triplet, i.e., into REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY
(and it is renamed to REPLY.CHECK_REPLY_MAGIC). The receive stage of the
second triplet is moved to "states-reply.c", and renamed to
REPLY.RECV_STRUCTURED_REMAINING. The last (parse) stage of the second
triplet is kept in "states-reply-structured.c", but renamed to
REPLY.STRUCTURED_REPLY.START.
- Thus "states-reply-structured.c" loses two states (prepare and
receive); the first is absorbed by "states-reply.c" into an existing
state, and the second is gained by "states-reply.c" as a new state.
I agree this is an improvement, because now we finish reading all reply
headers in a single C file.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
generator/state_machine.ml | 29 ++++++++-----------
generator/states-reply.c | 43 ++++++++++++++++++++++-------
generator/states-reply-structured.c | 26 -----------------
3 files changed, 44 insertions(+), 54 deletions(-)
diff --git a/generator/state_machine.ml b/generator/state_machine.ml
index 126159b9..b5485aec 100644
--- a/generator/state_machine.ml
+++ b/generator/state_machine.ml
@@ -769,8 +769,15 @@ and
State {
default_state with
- name = "CHECK_SIMPLE_OR_STRUCTURED_REPLY";
- comment = "Check if the reply is a simple or structured reply";
+ name = "CHECK_REPLY_MAGIC";
+ comment = "Check if the reply has expected magic";
+ external_events = [];
+ };
So this remains in "states-reply.c" and absorbs the original prep logic
of REPLY.STRUCTURED_REPLY.START, from "states-reply-structured.c".
+
+ State {
+ default_state with
+ name = "RECV_STRUCTURED_REMAINING";
+ comment = "Receiving the remaining part of a structured reply header";
external_events = [];
};
This is effectively moved from "states-reply-structured.c" (originally
REPLY.STRUCTURED_REPLY.RECV_REMAINING) to "states-reply.c".
@@ -804,28 +811,14 @@ and
};
]
-(* Receiving a structured reply from the server.
+(* Receiving a structured reply payload from the server.
* Implementation: generator/states-reply-structured.c
*)
and structured_reply_state_machine = [
State {
default_state with
name = "START";
- comment = "Prepare to receive the remaining part of a structured reply";
- external_events = [];
- };
-
Yes, moved & merged into CHECK_REPLY_MAGIC.
- State {
- default_state with
- name = "RECV_REMAINING";
- comment = "Receiving the remaining part of a structured reply";
- external_events = [];
- };
Yes, moved and renamed to RECV_STRUCTURED_REMAINING.
-
- State {
- default_state with
- name = "CHECK";
- comment = "Parse a structured reply from the server";
+ comment = "Start parsing a structured reply payload from the server";
external_events = [];
};
Stays in "states-reply-structured.c", renamed from CHECK to START.
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 2c77658b..87f17bae 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -64,11 +64,11 @@ 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, 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).
+ * check the magic in CHECK_REPLY_MAGIC 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),
OK, only the state name is updated, and the comment is reflowed.
@@ -113,11 +113,11 @@ REPLY.RECV_REPLY:
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return 0;
case 1: SET_NEXT_STATE (%.READY); return 0;
- case 0: SET_NEXT_STATE (%CHECK_SIMPLE_OR_STRUCTURED_REPLY);
+ case 0: SET_NEXT_STATE (%CHECK_REPLY_MAGIC);
}
return 0;
OK, state rename.
- REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
+ REPLY.CHECK_REPLY_MAGIC:
struct command *cmd;
uint32_t magic;
uint64_t cookie;
Ditto.
@@ -127,7 +127,18 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
SET_NEXT_STATE (%SIMPLE_REPLY.START);
}
else if (magic == NBD_STRUCTURED_REPLY_MAGIC) {
- SET_NEXT_STATE (%STRUCTURED_REPLY.START);
+ /* We've only read the bytes that fill simple_reply. The
+ * structured_reply 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 ==
+ 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;
This is being absorbed from REPLY.STRUCTURED_REPLY.START. The comment's
wording is slightly updated. OK.
The code is identical.
+ SET_NEXT_STATE (%RECV_STRUCTURED_REMAINING);
Having dealt with the "prepare" stage right here, we can now read the
remaining bytes; that gets renamed from
REPLY.STRUCTURED_REPLY.RECV_REMAINING, and moved to this file.
}
else {
/* We've probably lost synchronization. */
@@ -141,8 +152,9 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
return 0;
}
- /* NB: This works for both simple and structured replies because the
- * handle (our cookie) is stored at the same offset. See the
+ /* NB: This works for both simple and structured replies, even
+ * though we haven't finished reading the structured header yet,
+ * because the cookie is stored at the same offset. See the
* STATIC_ASSERT above in state REPLY.START that confirmed this.
*/
h->chunks_received++;
Good!
@@ -157,6 +169,17 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
h->reply_cmd = cmd;
return 0;
+ REPLY.RECV_STRUCTURED_REMAINING:
+ switch (recv_into_rbuf (h)) {
+ case -1: SET_NEXT_STATE (%.DEAD); return 0;
+ case 1:
+ save_reply_state (h);
+ SET_NEXT_STATE (%.READY);
+ return 0;
+ case 0: SET_NEXT_STATE (%STRUCTURED_REPLY.START);
+ }
+ return 0;
+
REPLY.FINISH_COMMAND:
struct command *prev_cmd, *cmd;
uint64_t cookie;
Moved nearly identically from REPLY.STRUCTURED_REPLY.RECV_REMAINING. The
state itself is renamed, and the state we jump to (for parsing) is also
renamed. Good.
"Nearly" identically: the code movement sneakily fixes a whitespace
error in the original! :) Double space after "case 0:".
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 205a236d..3a7a03fd 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -46,32 +46,6 @@ structured_reply_in_bounds (uint64_t offset, uint32_t length,
STATE_MACHINE {
REPLY.STRUCTURED_REPLY.START:
- /* 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.
- */
- 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);
- return 0;
-
- REPLY.STRUCTURED_REPLY.RECV_REMAINING:
- switch (recv_into_rbuf (h)) {
- case -1: SET_NEXT_STATE (%.DEAD); return 0;
- case 1:
- save_reply_state (h);
- SET_NEXT_STATE (%.READY);
- return 0;
- case 0: SET_NEXT_STATE (%CHECK);
- }
- return 0;
-
- REPLY.STRUCTURED_REPLY.CHECK:
struct command *cmd = h->reply_cmd;
uint16_t flags, type;
uint32_t length;
Yes.
Hellishly difficult to review, but correct, as far as I can tell, and
certainly an improvement for the codebase.
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
I'll continue with the rest of the series sometime later.
Thanks,
Laszlo
3:17 p.m.
New subject: [libnbd PATCH v4 2/4] generator: Finish parsing structured header in states-reply.c
On Fri, Jun 09, 2023 at 05:22:36PM +0200, Laszlo Ersek wrote:
On 6/9/23 04:17, Eric Blake wrote:
> Splitting the parse of a 20-byte structured reply header across two
> source files (16 bytes overlapping with simple replies in
> states-reply.c, the remaining 4 bytes in states-reply-structured.c) is
> confusing. The upcoming addition of extended headers will reuse the
> payload parsing portion of structured replies, but will parse its
> 32-byte header completely in states-reply.c. So it is better to have
> a single file in charge of parsing all three header sizes (once the
> third type is introduced), where we then farm out to the payload
> parsers.
"Farm out" is a transitive phrasal verb, but we don't have an object for
it. "Fan out" perhaps? (Or add an object.)
Fan out works for me (and yeah, I can see how this one is more of a
localized idiom than a common English phrase); mentioning the payload
parsers states-reply-simple and states-reply-structured by name can't
hurt either.
>
> To do that, rename REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY to
> REPLY.CHECK_REPLY_MAGIC and have it pick up the setup from
> REPLY.STRUCTURED_REPLY.START, rename REPLY
The last REPLY is superfluous
The wall of ALL CAPS makes it harder to read. Your rewording for the
commit message was very nice; I've lifted quite a bit of it.
> REPLY.STRUCTURED_REPLY.RECV_REMAINING to
> REPLY.RECV_STRUCTURED_REMAINING across files, and merge
> REPLY.STRUCTURED_REPLY.CHECK into what would otherwise be the
> now-empty REPLY.STRUCTURED_REPLY.START.
A different way to describe the same restructuring (I needed this
wording for explaining the code movement to myself):
- We have (prepare, receive, parse) triplets.
More generically, our state machine is separated between phases of
preparing h->rbuf/rlen (where to read to), then a re-entrant state
that reads into the buffer (can be re-started as many times when
partial reads block), then once h->rlen reaches zero post-process the
data. Sometimes, when we read into a buffer, we immediately parse it
in the same function; other times, we merely SET_NEXT to a parser
state to parse the data (part of it depends on how much code reuse we
want to have, and whether embedding the parser inside a
switch(recv_into_rbuf) is going to be confusing because the parser
itself also wants a switch, but C lacks Java's 'break label;'
construct). But yes, for structured headers, we do indeed start with
two state triples that break out those actions, and have some
flexibility to consolidate things or split out more states if it makes
the code easier to read.
- Before the patch, our starter triplet is (REPLY.START,
REPLY.RECV_REPLY, REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY). These are all
in "states-reply.c". In the last component of that triplet (the
"parse"
component), we branch either to SIMPLE_REPLY.START
(states-reply-simple.c) for actual parsing, or we start another triplet,
for the remaining bytes of a structured reply:
(REPLY.STRUCTURED_REPLY.START, REPLY.STRUCTURED_REPLY.RECV_REMAINING,
REPLY.STRUCTURED_REPLY.CHECK); these are all in
states-reply-structured.c
- The patch effectively redistributes the second triplet. The prepare
stage REPLY.STRUCTURED_REPLY.START is merged into the last (parse) stage
of the first triplet, i.e., into REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY
(and it is renamed to REPLY.CHECK_REPLY_MAGIC). The receive stage of the
second triplet is moved to "states-reply.c", and renamed to
REPLY.RECV_STRUCTURED_REMAINING. The last (parse) stage of the second
triplet is kept in "states-reply-structured.c", but renamed to
REPLY.STRUCTURED_REPLY.START.
Partly because the state machine generator requires that any
sub-hierarchy of states must have a START block (when a parent state
group wants to call into the subhierarchy, it must use the START
state).
- Thus "states-reply-structured.c" loses two states (prepare and
receive); the first is absorbed by "states-reply.c" into an existing
state, and the second is gained by "states-reply.c" as a new state.
I agree this is an improvement, because now we finish reading all reply
headers in a single C file.
[...]
Hellishly difficult to review, but correct, as far as I can tell, and
certainly an improvement for the codebase.
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks for taking the time on this one. I tweaked the commit message,
then pushed this as 2e34ceb8
I'll continue with the rest of the series sometime later.
Thanks,
Laszlo
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
9:17 p.m.
New subject: [libnbd PATCH v4 3/4] generator: Rename states-reply-structured to states-reply-chunk
Upcoming patches to add extended headers want to share the common
payload parser with structured replies. Renaming the file and the
associated states from "structured" to "chunk" makes it more obvious
that we will be sharing the code independent from the header style
parsed in the earlier REPLY portion of the state machine.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
generator/state_machine.ml | 26 +++++++++----------
generator/states-reply.c | 2 +-
...eply-structured.c => states-reply-chunk.c} | 22 ++++++++--------
generator/Makefile.am | 2 +-
4 files changed, 26 insertions(+), 26 deletions(-)
rename generator/{states-reply-structured.c => states-reply-chunk.c} (97%)
diff --git a/generator/state_machine.ml b/generator/state_machine.ml
index b5485aec..3a912508 100644
--- a/generator/state_machine.ml
+++ b/generator/state_machine.ml
@@ -782,7 +782,7 @@ and
};
Group ("SIMPLE_REPLY", simple_reply_state_machine);
- Group ("STRUCTURED_REPLY", structured_reply_state_machine);
+ Group ("CHUNK_REPLY", chunk_reply_state_machine);
State {
default_state with
@@ -812,76 +812,76 @@ and
]
(* Receiving a structured reply payload from the server.
- * Implementation: generator/states-reply-structured.c
+ * Implementation: generator/states-reply-chunk.c
*)
-and structured_reply_state_machine = [
+and chunk_reply_state_machine = [
State {
default_state with
name = "START";
- comment = "Start parsing a structured reply payload from the server";
+ comment = "Start parsing a chunk reply payload from the server";
external_events = [];
};
State {
default_state with
name = "RECV_ERROR";
- comment = "Receive a structured reply error header";
+ comment = "Receive a chunk reply error header";
external_events = []
};
State {
default_state with
name = "RECV_ERROR_MESSAGE";
- comment = "Receive a structured reply error message";
+ comment = "Receive a chunk reply error message";
external_events = [];
};
State {
default_state with
name = "RECV_ERROR_TAIL";
- comment = "Receive a structured reply error tail";
+ comment = "Receive a chunk reply error tail";
external_events = [];
};
State {
default_state with
name = "RECV_OFFSET_DATA";
- comment = "Receive a structured reply offset-data header";
+ comment = "Receive a chunk reply offset-data header";
external_events = [];
};
State {
default_state with
name = "RECV_OFFSET_DATA_DATA";
- comment = "Receive a structured reply offset-data block of data";
+ comment = "Receive a chunk reply offset-data block of data";
external_events = [];
};
State {
default_state with
name = "RECV_OFFSET_HOLE";
- comment = "Receive a structured reply offset-hole header";
+ comment = "Receive a chunk reply offset-hole header";
external_events = [];
};
State {
default_state with
name = "RECV_BS_ENTRIES";
- comment = "Receive a structured reply block-status payload";
+ comment = "Receive a chunk reply block-status payload";
external_events = [];
};
State {
default_state with
name = "RESYNC";
- comment = "Ignore payload of an unexpected structured reply";
+ comment = "Ignore payload of an unexpected chunk reply";
external_events = [];
};
State {
default_state with
name = "FINISH";
- comment = "Finish receiving a structured reply";
+ comment = "Finish receiving a chunk reply";
external_events = [];
};
]
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 87f17bae..bd6336a8 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -176,7 +176,7 @@ REPLY.RECV_STRUCTURED_REMAINING:
save_reply_state (h);
SET_NEXT_STATE (%.READY);
return 0;
- case 0: SET_NEXT_STATE (%STRUCTURED_REPLY.START);
+ case 0: SET_NEXT_STATE (%CHUNK_REPLY.START);
}
return 0;
diff --git a/generator/states-reply-structured.c b/generator/states-reply-chunk.c
similarity index 97%
rename from generator/states-reply-structured.c
rename to generator/states-reply-chunk.c
index 3a7a03fd..94f3a8ae 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-chunk.c
@@ -16,7 +16,7 @@
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
*/
-/* State machine for parsing structured replies from the server. */
+/* State machine for parsing structured reply chunk payloads from the server. */
#include <stdbool.h>
#include <stddef.h>
@@ -45,7 +45,7 @@ structured_reply_in_bounds (uint64_t offset, uint32_t length,
}
STATE_MACHINE {
- REPLY.STRUCTURED_REPLY.START:
+ REPLY.CHUNK_REPLY.START:
struct command *cmd = h->reply_cmd;
uint16_t flags, type;
uint32_t length;
@@ -145,7 +145,7 @@ REPLY.STRUCTURED_REPLY.START:
SET_NEXT_STATE (%RESYNC);
return 0;
- REPLY.STRUCTURED_REPLY.RECV_ERROR:
+ REPLY.CHUNK_REPLY.RECV_ERROR:
struct command *cmd = h->reply_cmd;
uint32_t length, msglen, error;
@@ -184,7 +184,7 @@ REPLY.STRUCTURED_REPLY.RECV_ERROR:
SET_NEXT_STATE (%RESYNC);
return 0;
- REPLY.STRUCTURED_REPLY.RECV_ERROR_MESSAGE:
+ REPLY.CHUNK_REPLY.RECV_ERROR_MESSAGE:
uint32_t length, msglen;
uint16_t type;
@@ -226,7 +226,7 @@ REPLY.STRUCTURED_REPLY.RECV_ERROR_MESSAGE:
}
return 0;
- REPLY.STRUCTURED_REPLY.RECV_ERROR_TAIL:
+ REPLY.CHUNK_REPLY.RECV_ERROR_TAIL:
struct command *cmd = h->reply_cmd;
uint32_t error;
uint16_t type;
@@ -283,7 +283,7 @@ REPLY.STRUCTURED_REPLY.RECV_ERROR_TAIL:
}
return 0;
- REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA:
+ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA:
struct command *cmd = h->reply_cmd;
uint64_t offset;
uint32_t length;
@@ -322,7 +322,7 @@ REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA:
}
return 0;
- REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA:
+ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA_DATA:
struct command *cmd = h->reply_cmd;
uint64_t offset;
uint32_t length;
@@ -353,7 +353,7 @@ REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA:
}
return 0;
- REPLY.STRUCTURED_REPLY.RECV_OFFSET_HOLE:
+ REPLY.CHUNK_REPLY.RECV_OFFSET_HOLE:
struct command *cmd = h->reply_cmd;
uint64_t offset;
uint32_t length;
@@ -403,7 +403,7 @@ REPLY.STRUCTURED_REPLY.RECV_OFFSET_HOLE:
}
return 0;
- REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES:
+ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
struct command *cmd = h->reply_cmd;
uint32_t length;
size_t i;
@@ -457,7 +457,7 @@ REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES:
}
return 0;
- REPLY.STRUCTURED_REPLY.RESYNC:
+ REPLY.CHUNK_REPLY.RESYNC:
struct command *cmd = h->reply_cmd;
uint16_t type;
uint32_t length;
@@ -491,7 +491,7 @@ REPLY.STRUCTURED_REPLY.RESYNC:
}
return 0;
- REPLY.STRUCTURED_REPLY.FINISH:
+ REPLY.CHUNK_REPLY.FINISH:
uint16_t flags;
flags = be16toh (h->sbuf.sr.structured_reply.flags);
diff --git a/generator/Makefile.am b/generator/Makefile.am
index 91dbde5c..39285e82 100644
--- a/generator/Makefile.am
+++ b/generator/Makefile.am
@@ -37,8 +37,8 @@ states_code = \
states-newstyle-opt-structured-reply.c \
states-newstyle.c \
states-oldstyle.c \
+ states-reply-chunk.c \
states-reply-simple.c \
- states-reply-structured.c \
states-reply.c \
$(NULL)
--
2.40.1
3:34 p.m.
New subject: [libnbd PATCH v4 3/4] generator: Rename states-reply-structured to states-reply-chunk
On Thu, Jun 08, 2023 at 09:17:37PM -0500, Eric Blake wrote:
Upcoming patches to add extended headers want to share the common
payload parser with structured replies. Renaming the file and the
associated states from "structured" to "chunk" makes it more obvious
that we will be sharing the code independent from the header style
parsed in the earlier REPLY portion of the state machine.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
generator/state_machine.ml | 26 +++++++++----------
generator/states-reply.c | 2 +-
...eply-structured.c => states-reply-chunk.c} | 22 ++++++++--------
generator/Makefile.am | 2 +-
4 files changed, 26 insertions(+), 26 deletions(-)
rename generator/{states-reply-structured.c => states-reply-chunk.c} (97%)
I'm thinking of squashing in:
diff --git c/lib/internal.h w/lib/internal.h
index 1e8d02f1..4b0043b3 100644
--- c/lib/internal.h
+++ w/lib/internal.h
@@ -252,10 +252,10 @@ struct nbd_handle {
} hdr;
union {
uint64_t align_; /* Start reply.payload on an 8-byte alignment */
- struct nbd_structured_reply_offset_data offset_data;
- struct nbd_structured_reply_offset_hole offset_hole;
+ struct nbd_chunk_offset_data offset_data;
+ struct nbd_chunk_offset_hole offset_hole;
struct {
- struct nbd_structured_reply_error error;
+ struct nbd_chunk_error error;
char msg[NBD_MAX_STRING]; /* Common to all error types */
uint64_t offset; /* Only used for NBD_REPLY_TYPE_ERROR_OFFSET */
} NBD_ATTRIBUTE_PACKED error;
diff --git c/lib/nbd-protocol.h w/lib/nbd-protocol.h
index 50275dcd..fef19f2c 100644
--- c/lib/nbd-protocol.h
+++ w/lib/nbd-protocol.h
@@ -211,20 +211,20 @@ struct nbd_structured_reply {
uint16_t flags; /* NBD_REPLY_FLAG_* */
uint16_t type; /* NBD_REPLY_TYPE_* */
uint64_t cookie; /* Opaque handle. */
- uint32_t length; /* Length of payload which follows. */
+ uint32_t length; /* Length of following nbd_chunk_* payload. */
} NBD_ATTRIBUTE_PACKED;
-struct nbd_structured_reply_offset_data {
+struct nbd_chunk_offset_data {
uint64_t offset; /* offset */
/* Followed by data. */
} NBD_ATTRIBUTE_PACKED;
-struct nbd_structured_reply_offset_hole {
+struct nbd_chunk_offset_hole {
uint64_t offset;
uint32_t length; /* Length of hole. */
} NBD_ATTRIBUTE_PACKED;
-struct nbd_structured_reply_error {
+struct nbd_chunk_error {
uint32_t error; /* NBD_E* error number */
uint16_t len; /* Length of human readable error. */
/* Followed by human readable error string, and possibly more structure. */
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
8:56 a.m.
New subject: [libnbd PATCH v4 3/4] generator: Rename states-reply-structured to states-reply-chunk
On 6/12/23 22:34, Eric Blake wrote:
On Thu, Jun 08, 2023 at 09:17:37PM -0500, Eric Blake wrote:
> Upcoming patches to add extended headers want to share the common
> payload parser with structured replies. Renaming the file and the
> associated states from "structured" to "chunk" makes it more
obvious
> that we will be sharing the code independent from the header style
> parsed in the earlier REPLY portion of the state machine.
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
> generator/state_machine.ml | 26 +++++++++----------
> generator/states-reply.c | 2 +-
> ...eply-structured.c => states-reply-chunk.c} | 22 ++++++++--------
> generator/Makefile.am | 2 +-
> 4 files changed, 26 insertions(+), 26 deletions(-)
> rename generator/{states-reply-structured.c => states-reply-chunk.c} (97%)
I'm thinking of squashing in:
diff --git c/lib/internal.h w/lib/internal.h
index 1e8d02f1..4b0043b3 100644
--- c/lib/internal.h
+++ w/lib/internal.h
@@ -252,10 +252,10 @@ struct nbd_handle {
} hdr;
union {
uint64_t align_; /* Start reply.payload on an 8-byte alignment */
- struct nbd_structured_reply_offset_data offset_data;
- struct nbd_structured_reply_offset_hole offset_hole;
+ struct nbd_chunk_offset_data offset_data;
+ struct nbd_chunk_offset_hole offset_hole;
struct {
- struct nbd_structured_reply_error error;
+ struct nbd_chunk_error error;
char msg[NBD_MAX_STRING]; /* Common to all error types */
uint64_t offset; /* Only used for NBD_REPLY_TYPE_ERROR_OFFSET */
} NBD_ATTRIBUTE_PACKED error;
diff --git c/lib/nbd-protocol.h w/lib/nbd-protocol.h
index 50275dcd..fef19f2c 100644
--- c/lib/nbd-protocol.h
+++ w/lib/nbd-protocol.h
@@ -211,20 +211,20 @@ struct nbd_structured_reply {
uint16_t flags; /* NBD_REPLY_FLAG_* */
uint16_t type; /* NBD_REPLY_TYPE_* */
uint64_t cookie; /* Opaque handle. */
- uint32_t length; /* Length of payload which follows. */
+ uint32_t length; /* Length of following nbd_chunk_* payload. */
} NBD_ATTRIBUTE_PACKED;
-struct nbd_structured_reply_offset_data {
+struct nbd_chunk_offset_data {
uint64_t offset; /* offset */
/* Followed by data. */
} NBD_ATTRIBUTE_PACKED;
-struct nbd_structured_reply_offset_hole {
+struct nbd_chunk_offset_hole {
uint64_t offset;
uint32_t length; /* Length of hole. */
} NBD_ATTRIBUTE_PACKED;
-struct nbd_structured_reply_error {
+struct nbd_chunk_error {
uint32_t error; /* NBD_E* error number */
uint16_t len; /* Length of human readable error. */
/* Followed by human readable error string, and possibly more structure. */
For both (squashed):
Acked-by: Laszlo Ersek <lersek(a)redhat.com>
9:17 p.m.
New subject: [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>
---
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
8 a.m.
New subject: [libnbd PATCH v4 4/4] internal: Refactor layout of replies in sbuf
On Thu, Jun 08, 2023 at 09:17:38PM -0500, 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.
[...]
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>
---
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(-)
Here's what I'm leaning towards squashing in, now that patch 2/4
landed as 2e34ceb8.
diff --git a/generator/states-reply.c b/generator/states-reply.c
index af5f6135..0d200513 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -19,6 +19,11 @@
#include <assert.h>
#include <stddef.h>
+#define ASSERT_MEMBER_ALIAS(type, member_a, member_b) \
+ STATIC_ASSERT (offsetof (type, member_a) == offsetof (type, member_b) && \
+ sizeof ((type *)NULL)->member_a == \
+ sizeof ((type *)NULL)->member_b, member_alias)
+
/* State machine for receiving reply messages from the server.
*
* Note that we never block while in this sub-group. If there is
@@ -70,12 +75,10 @@ REPLY.START:
* structs (an intentional design decision in the NBD spec when
* structured replies were added).
*/
- 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_MEMBER_ALIAS (union reply_header, simple.magic, magic);
+ ASSERT_MEMBER_ALIAS (union reply_header, simple.cookie, cookie);
+ ASSERT_MEMBER_ALIAS (union reply_header, structured.magic, magic);
+ ASSERT_MEMBER_ALIAS (union reply_header, structured.cookie, cookie);
assert (h->reply_cmd == NULL);
assert (h->rlen == 0);
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
8:57 a.m.
New subject: [libnbd PATCH v4 4/4] internal: Refactor layout of replies in sbuf
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 <eblake(a)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 <lersek(a)redhat.com>
8:31 p.m.
New subject: [libnbd PATCH v4 4/4] internal: Refactor layout of replies in sbuf
On Tue, Jun 20, 2023 at 03:57:04PM +0200, Laszlo Ersek wrote:
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.
>
[...]
Acked-by: Laszlo Ersek <lersek(a)redhat.com>
Patches 1 and 2 were pushed earlier; 3 and 4 are now in as
20dceca4..33a47171
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
313
days inactive
346
days old
16 comments
3 participants
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones