Re: [Libguestfs] [libnbd PATCH v3 05/22] states: Prepare to receive 64-bit replies

[Laszlo Ersek]

On 6/1/23 15:00, Eric Blake wrote:
> On Thu, Jun 01, 2023 at 11:04:05AM +0200, Laszlo Ersek wrote:

>> Probably best to reorder the files in this patch for review:
> 
> I see what you mean: because of the state hierarchy, it is probably
> worth tweaking the git orderfile to favor files nearer the top of the
> state tree first, rather than strict alphabetical ordering of *.c.  I
> may just push a patch for that without needing review, as it doesn't
> affect library semantics at all.

Ouch, I'm sorry, I meant that *I* should rearrange the hunks in the
patch for review! I didn't mean to ask you for any orderfile changes! :)

Of course if you can do that: high five! :)

> 
> ...
>>> +++ b/generator/states-reply.c
>>> @@ -62,15 +62,19 @@  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.
>>> +  /* With extended headers, there is only one size to read, so we can do it
>>> +   * all in one syscall.  But for older structured replies, we don't know 
>>> if
>>> +   * we have a simple or structured reply until we read the magic number,
>>> +   * requiring a two-part read with CHECK_SIMPLE_OR_STRUCTURED_REPLY below.
>>>     */
>>>    assert (h->reply_cmd == NULL);
>>>    assert (h->rlen == 0);
>>>
>>>    h->rbuf = &h->sbuf.reply.hdr;
>>> -  h->rlen = sizeof h->sbuf.reply.hdr.simple;
>>> +  if (h->extended_headers)
>>> +    h->rlen = sizeof h->sbuf.reply.hdr.extended;
>>> +  else
>>> +    h->rlen = sizeof h->sbuf.reply.hdr.simple;
>>>
>>>    r = h->sock->ops->recv (h, h->sock, h->rbuf, h->rlen);
>>>    if (r == -1) {
>>
>> The comment "This works because the structured_reply header is larger"
>> is being removed, which makes the non-ext branch even less
>> comprehensible than before.
>>
>> (2) Can we add the STATIC_ASSERT() here, stating that "sizeof simple" is
>> smaller than "sizeof structured"?
> 
> Yep, I'm already rebasing onto some additional asserts along those
> lines, based on your reviews earlier in the series.
> 
>>
>>> @@ -116,16 +120,22 @@  REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY:
>>>    uint64_t cookie;
>>>
>>>    magic = be32toh (h->sbuf.reply.hdr.simple.magic);
>>> -  if (magic == NBD_SIMPLE_REPLY_MAGIC) {
>>> +  switch (magic) {
>>> +  case NBD_SIMPLE_REPLY_MAGIC:
>>> +    if (h->extended_headers)
>>> +      goto invalid;
>>>      SET_NEXT_STATE (%SIMPLE_REPLY.START);
>>> -  }
>>> -  else if (magic == NBD_STRUCTURED_REPLY_MAGIC) {
>>> +    break;
>>> +  case NBD_STRUCTURED_REPLY_MAGIC:
>>> +  case NBD_EXTENDED_REPLY_MAGIC:
>>> +    if ((magic == NBD_STRUCTURED_REPLY_MAGIC) == h->extended_headers)
>>
>> Heh, I love this ((a == b) == c) "equivalence" form! :)
> 
> I still do a double-take every time I see 'a < b < c' in languages
> (like python) that support it as shorthand for 'a < b && b < c'.  Even
> weirder is when you see 'a < b > c'.  But yes, C's non-transitive ==
> can be a surprise for the uninitiated.
> 
>>
>>> +      goto invalid;
>>>      SET_NEXT_STATE (%STRUCTURED_REPLY.START);
>>> -  }
>>> -  else {
>>> -    /* We've probably lost synchronization. */
>>> -    SET_NEXT_STATE (%.DEAD);
>>> -    set_error (0, "invalid reply magic 0x%" PRIx32, magic);
>>> +    break;
>>> +  default:
>>> +  invalid:
>>> +    SET_NEXT_STATE (%.DEAD); /* We've probably lost synchronization. */
>>> +    set_error (0, "invalid or unexpected reply magic 0x%" PRIx32, magic);
>>>  #if 0 /* uncomment to see desynchronized data */
>>>      nbd_internal_hexdump (&h->sbuf.reply.hdr.simple,
>>>                            sizeof (h->sbuf.reply.hdr.simple),
>>
>> My apologies, but I find *this* placement of "invalid" terrible. I
>> thought the "goto invalid" statements were jumping to the end of the
>> function. Now I see they jump effectively to another case label.
>>
>> (3) How about this (on top of your patch, to be squashed):
> 
> That part of the patch pre-dates other reviews I've seen from you on
> the same topic (this patch series has been percolating on my local
> builds for a long time), so I'm not surprised by your request, and I'm
> happy to squash in your improvement.  I may have copied from other
> similar code in the generator/states-*.c files, if so, I'll probably
> do a separate cleanup patch first.
> 
>>
>> ... At the same time, even with this "cleanup" for the labels, I find it
>> relatively confusing (albeit correct, as far as I can tell!) that in
>> REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY, we first check the magic
>> number(s) and *then* whether we negotiated extended headers, whereas in
>> all the other state handlers, the order (= condition nesting) is the
>> opposite: we first check extended headers, and deal with any other
>> objects second. This makes the assert()s in REPLY.STRUCTURED_REPLY.START
>> especially tricky to demonstrate -- I think I've managed to do it, it
>> looks correct, it's just hard. So that's a half- (or quarter-)hearted
>> proposal to investigate how REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY
>> looked if there too, "extended_headers" were the outer check. Anyway, I
>> don't feel strongly about this. :)
>>
>> (4) Still in REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY, we have a comment
>> saying
>>
>>    /* NB: This works for both simple and structured replies because the
>>     * handle (our cookie) is stored at the same offset.
>>     */
>>
>> The code under it applies to extended headers too, so the comment should
>> be updated.
>>
>> (There's a similar comment in REPLY.FINISH_COMMAND, too; I'm not sure if
>> that state is relevant for extended headers.)
> 
> Indeed, when rebasing, I need to remember to grep (to cover comments)
> rather than merely relying on the compiler (which only covers code)
> for renames.
> 
>>
>> Then:
>>
>>> diff --git a/generator/states-reply-structured.c 
>>> b/generator/states-reply-structured.c
>>> index df509216..c53aed7b 100644
>>> --- a/generator/states-reply-structured.c
>>> +++ b/generator/states-reply-structured.c
>>> @@ -45,14 +45,20 @@ 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.
>>> +  /* If we have extended headers, we've already read the entire header.
>>> +   * Otherwise, we've only read enough for a simple_reply; since structured
>>> +   * replies are longer, read the remaining part.
>>>     */
>>
>> (5) Ah, the word "simple_reply", which this change preserves, is a
>> leftover. It should be updated in patch#2, "internal: Refactor layout of
>> replies in sbuf", where the "simple_reply" field is replaced with
>> "reply.hdr.simple" in "sbuf".
>>
>> I guess I didn't notice this omission in patch#2 because the context
>> there only shows the "so read the remaining part" line of the comment,
>> and "simple_reply" is on the preceding line.
>>
>> And then this patch too should refer to "reply.hdr.simple", in the new
>> comment.
>>
>> Alternatively, perhaps use the type name (struct tag, actually):
>> "nbd_simple_reply".
> 
> Will fix in the appropriate place(s).
> 
>>
>>> -  h->rbuf = &h->sbuf;
>>> -  h->rbuf = (char *)h->rbuf + 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_REMAINING);
>>> +  if (h->extended_headers) {
>>> +    assert (h->rbuf == sizeof h->sbuf.reply.hdr.extended + 
>>> (char*)&h->sbuf);
>>> +    SET_NEXT_STATE (%CHECK);
>>> +  }
>>> +  else {
>>> +    assert (h->rbuf == sizeof h->sbuf.reply.hdr.simple + (char*)&h->sbuf);
>>> +    h->rlen = sizeof h->sbuf.reply.hdr.structured -
>>> +      sizeof h->sbuf.reply.hdr.simple;
>>> +    SET_NEXT_STATE (%RECV_REMAINING);
>>> +  }
>>>    return 0;
>>>
>>>   REPLY.STRUCTURED_REPLY.RECV_REMAINING:
>>
>> This looks OK, but can we make it less verbose? How about:
>>
>> /----------------------
>> | diff --git a/lib/internal.h b/lib/internal.h
>> | index e4e21a4d1ffa..a630b2511ff7 100644
>> | --- a/lib/internal.h
>> | +++ b/lib/internal.h
>> | @@ -240,7 +240,7 @@ struct nbd_handle {
>> |      } or;
>> |      struct nbd_export_name_option_reply export_name_reply;
>> |      struct {
>> | -      union {
>> | +      union reply_header {
> 
> Oh cool.  I keep forgetting that you can declare a type name usable at
> the top level even while declaring a nested struct.  Yeah, doing that
> probably has some nice line length improvements.
> 
>> |          struct nbd_simple_reply simple;
>> |          struct nbd_structured_reply structured;
>> |          struct nbd_extended_reply extended;
>> | diff --git a/generator/states-reply-structured.c 
>> b/generator/states-reply-structured.c
>> | index c53aed7bb859..852cb5b6053c 100644
>> | --- a/generator/states-reply-structured.c
>> | +++ b/generator/states-reply-structured.c
>> | @@ -49,14 +49,14 @@  REPLY.STRUCTURED_REPLY.START:
>> |     * Otherwise, we've only read enough for a simple_reply; since 
>> structured
>> |     * replies are longer, read the remaining part.
>> |     */
>> | +  union reply_header *hdr = &h->sbuf.reply.hdr;
>> |    if (h->extended_headers) {
>> | -    assert (h->rbuf == sizeof h->sbuf.reply.hdr.extended + 
>> (char*)&h->sbuf);
>> | +    assert (h->rbuf == sizeof hdr->extended + (char*)&h->sbuf);
>> |      SET_NEXT_STATE (%CHECK);
>> |    }
>> |    else {
>> | -    assert (h->rbuf == sizeof h->sbuf.reply.hdr.simple + (char*)&h->sbuf);
>> | -    h->rlen = sizeof h->sbuf.reply.hdr.structured -
>> | -      sizeof h->sbuf.reply.hdr.simple;
>> | +    assert (h->rbuf == sizeof hdr->simple + (char*)&h->sbuf);
>> | +    h->rlen = sizeof hdr->structured - sizeof hdr->simple;
>> |      SET_NEXT_STATE (%RECV_REMAINING);
>> |    }
>> |    return 0;
>> \----------------------
>>
>> Anyway, feel free to ignore this -- I can see two counter-arguments
>> myself:
>>
>> - the rest of the code remains just as verbose,
>>
> 
> Not necessarily - once I add in more STATIC_ASSERTs, being able to
> refer to individual nested types without having to go all the way
> through struct nbd_handle or union sbuf will be shorter.
> 
>> - one could argue that the addition
>>
>>     sizeof hdr->simple + (char*)&h->sbuf
>>
>>   is *more* confusing than
>>
>>     sizeof h->sbuf.reply.hdr.simple + (char*)&h->sbuf
> 
> That argument still remains - it's easier to see that we expect a
> particular offset from sbuf when the use of sbuf in the offset
> calculation is not hidden several lines away in the intermediate
> initialization.   I'll think about it for this line.
> 
>>
>> Then:
>>
>>> @@ -69,11 +75,18 @@  REPLY.STRUCTURED_REPLY.RECV_REMAINING:
>>>   REPLY.STRUCTURED_REPLY.CHECK:
>>>    struct command *cmd = h->reply_cmd;
>>>    uint16_t flags, type;
>>> -  uint32_t length;
>>> +  uint64_t length;
>>> +  uint64_t offset = -1;
>>
>> (6) I disagree with initializing the local variable "offset" here.
>>
>> Below (in the rest of REPLY.STRUCTURED_REPLY.CHECK), we only read
>> "offset" back if "extended_headers" is set. But if "extended_headers" is
>> set, we also store a value to "offset", before the read.
>>
>> Initializing "offset" to -1 suggests that the code might otherwise read
>> an indeterminate value from "offset" -- but that's not the case.
> 
> I'll have to double-check; I thought that offset was read even for
> structured replies (where there really isn't an offset read from the
> wire), but my rebasing efforts may have changed that.
> 
>>
>>>
>>> +  assert (cmd);
>>
>> (7) What guarantees this?
>>
>> Is it the lookup code at the end of
>> REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY?
>>
>> But that code can set "reply_cmd" to NULL, in case the cookie is not
>> found; and the cookie lookup there defers to FINISH for diagnosing the
>> unassociated reply.
> 
> Hmmm.  Reading ahead, I see what you wrote in (9).  This may have been
> something I copied from other states, but I'll have to double check
> whether it still makes sense.
> 
>>
>>>    flags = be16toh (h->sbuf.reply.hdr.structured.flags);
>>>    type = be16toh (h->sbuf.reply.hdr.structured.type);
>>> -  length = be32toh (h->sbuf.reply.hdr.structured.length);
>>> +  if (h->extended_headers) {
>>> +    length = be64toh (h->sbuf.reply.hdr.extended.length);
>>> +    offset = be64toh (h->sbuf.reply.hdr.extended.offset);
>>> +  }
>>> +  else
>>> +    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
>>> @@ -83,13 +96,18 @@  REPLY.STRUCTURED_REPLY.CHECK:
>>>     * not worth keeping the connection alive.
>>>     */
>>>    if (length > MAX_REQUEST_SIZE + sizeof 
>>> h->sbuf.reply.payload.offset_data) {
>>> -    set_error (0, "invalid server reply length %" PRIu32, length);
>>> +    set_error (0, "invalid server reply length %" PRIu64, length);
>>>      SET_NEXT_STATE (%.DEAD);
>>>      return 0;
>>>    }
>>> +  /* For convenience, we now normalize extended replies into compact,
>>> +   * doable since we validated that payload length fits in 32 bits.
>>> +   */
>>> +  h->sbuf.reply.hdr.structured.length = length;
>>
>> (8) I'm very confused by this. For an extended reply, this will
>> overwrite the "offset" field.
> 
> At one point, I considered normalizing in the opposite direction (take
> structured replies and widen them into the extended header form); it
> required touching more lines of code, so I didn't pursue it at the
> time.  But I can see how compressing things down (intentionally
> throwing away information we will no longer reference) can be
> confusing without good comments, so at a minimum, I will be adding
> comments, if not outright going with the widening rather than
> narrowing approach.
> 
>>
>> I understand we have stolen that field already, above; but that's little
>> solace, especially without specific comments.
>>
>>>
>>>    /* Skip an unexpected structured reply, including to an unknown cookie. 
>>> */
>>> -  if (cmd == NULL || !h->structured_replies)
>>> +  if (cmd == NULL || !h->structured_replies ||
>>> +      (h->extended_headers && offset != cmd->offset))
>>>      goto resync;
>>>
>>>    switch (type) {
>>
>> (9) Preserving the (cmd == NULL) sub-condition, plus the reference to
>> "unkown cookie" in the comment, looks like a logic bug to me: it can
>> never evaluate to "true", given the assert() I'm questioning above at
>> (7).
>>
>> Alternatively, the assert() is wrong.
> 
> Off-hand, I think the assert is correct and I do have a dead condition
> here; but I'll have to convince myself of that.  Since the condition
> is pre-existing, it may be worth a separate patch adding just the
> assertion and removing the 'cmd == NULL' check here, if it is correct.
> 
>>
>> (10) The comment only talks about "unexpected structured reply", but the
>> condition now deals with extended headers too, and I don't know how
>> those relate to each other.
>>
>> What I'm trying to express is that
>>
>> (a) checking "structured_replies" *here*, but
>>
>> (b) checking "extended_headers" against magic numbers in
>> REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY,
>>
>> at the same time, seems inconsistent. If we get a structured reply after
>> *not* having negotiated them, we should be able to catch that in the
>> exact same location -- i.e., where we check magic numbers -- where we
>> can also realize that we're getting an extended reply in spite of *not*
>> having negotiated *those*.
>>
>> In other words, the treatment differs, and I don't know why: if we get
>> an unexpected structured reply, we skip it and "resync", whereas if we
>> get an unexpected extended reply, we move to the DEAD state.
>>
>> I think it's fine to skip replies that refer to unknown cookies, but
>> *reply format* violations should be fatal.
> 
> I think this all stems from me trying to be as generous as possible
> with bad servers: if we get a wrong reply type per the protocol but it
> is a reply type we know how to handle, AND we haven't consumed too
> many bytes for the size of that reply, then we can tolerate the
> server's bug.  If it is the wrong type, and we already read too many
> bytes for what the right type would be, we can't (easily) undo the
> fact that we read too much.
> 
> If we did not negotiate extended headers, we start by reading the
> length of a simple header; if the magic says it was instead an
> extended header, we can tolerate the server's mistake by reading the
> rest of that packet.  But I definitely see your point about
> consolidating the decisions to be local to one state, rather than
> split across two files, and aiming for consistent handling.

Right, I found your note about commit b31e7bac in patch#6, and I found
it hard to agree with the motivation behind it :) When reading patch#6,
I immediately felt it was somehow related to my point (10) here, under
patch#5. I couldn't figure out how exactly though, as the affected files
were different!

> 
> I'm liking your idea earlier in the series about reworking the state
> engine to FULLY parse the header (whether simple, structured, or
> extended) in states-reply.c, and only then move to
> states-reply-structured.c if the header was structured or extended,
> rather than splitting the header parse across two files.

I don't want to generate unnecessary work for you, but you too might
find future maintenance easier this way. From my perspective, I already
have to ping-pong between these two C files; not easy. :)

> 
>>
>>
>> Then, this is not a request for an update, just a comment: I don't
>> understand why the spec introduced the "offset" field at all. It does
>> not seem to carry additional information beyond the cookie. So we can
>> certainly check that the response's offset matches the command's offset
>> (the code looks OK), but the larger purpose is unclear. (And this is
>> probably also the reason why you get away with corrupting
>> "nbd_extended_reply.offset" at (8) -- the field is never again needed,
>> beyond the sanity check performed here.)
> 
> The evolution of that field: in my first draft, I wanted power-of-2
> sizing to the reply header (both request and reply having a header of
> exactly 32 bytes); this left dead space in the reply packet, which I
> originally tried to mandate to be zero-filled.  But when looking at
> the types again, I noticed that if the 'offset' field occupies the
> same relative place in the request and reply struct, a server can
> rewrite the reply in the same memory as it had saved for the request
> (I don't know that I actually implemented it that way in qemu as
> server, but it is possible for some other server).
> 
> There's also NBD_REPLY_TYPE_OFFSET_DATA and NBD_REPLY_TYPE_OFFSET_HOLE
> (used in response to NBD_CMD_READ), which each return an offset field
> in order to handle the case where a single client read request was
> split across multiple server replies.  When we first specified
> structured replies, the spec was ambiguous: if I issue
> NBD_CMD_READ(length=1k, offset=0) but the server replies with the pair
> OFFSET_DATA(length=512, offset=0) and OFFSET_HOLE(length=512,
> offset=512), it's obvious how to reconstruct the original buffer (read
> into the first 512 bytes, memset the second 512 bytes).  But if I
> issue NBD_CMD_READ(length=1k, offset=512), should the server reply
> OFFSET_DATA(length=512, offset=512) and OFFSET_HOLE(length=512,
> offset=1k) (that is, the reply offsets are absolute to the overall
> image), or with OFFSET_DATA(length=512, offset=0) and
> OFFSET_HOLE(length=512, offset=512) (that is, the reply offsets are
> relative to the start of the 1k buffer of the read request)?  The
> initial implementations of qemu and nbdkit disagreed, and the spec
> ended up settling in favor of the former (see nbd.git commit
> 587ba722).

Interesting: while reading your comparison, I immediately found the read
request relative reply offsets far-fetched and cumbersome, and the
absolute reply offsets "natural". :)

> 
> As a result, when receiving an absolute offset in a data reply, the
> client has to place the data into buffer + (reply_offset -
> request_offset) (that is, convert the server's absolute offset back
> into a buffer-relative offset), which requires knowing what the
> reqeust offset was; having the server return this offset in extended
> replies may make it easier to implement a client that doesn't have to
> store its request offset separately.  Since libnbd also handles
> non-extended headers (and has to hang on to the request offset for
> those), you are right that we end up merely validating that the
> server's reply offset was expected, and then throwing it away as it
> didn't add anything further than a validation that we haven't lost
> sync with the server.

Thanks for clarifying! So a less "legacy compatible" NBD client could
insist on extended headers (refuse working with servers not providing
extended headers), and then such a client might need the "offset" field
for real.

(On a tangent: I wonder if it were *secure* to trust the server's
"offset" field in the reply, for addressing client memory!)

> 
>>
>>> @@ -168,7 +186,7 @@  REPLY.STRUCTURED_REPLY.RECV_ERROR:
>>>      SET_NEXT_STATE (%.READY);
>>>      return 0;
>>>    case 0:
>>> -    length = be32toh (h->sbuf.reply.hdr.structured.length);
>>> +    length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */
>>>      assert (length >= sizeof h->sbuf.reply.payload.error.error.error);
>>>      assert (cmd);
>>>
>>> @@ -207,7 +225,7 @@  REPLY.STRUCTURED_REPLY.RECV_ERROR_MESSAGE:
>>>      SET_NEXT_STATE (%.READY);
>>>      return 0;
>>>    case 0:
>>> -    length = be32toh (h->sbuf.reply.hdr.structured.length);
>>> +    length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */
>>>      msglen = be16toh (h->sbuf.reply.payload.error.error.len);
>>>      type = be16toh (h->sbuf.reply.hdr.structured.type);
>>>
>>> @@ -307,7 +325,7 @@  REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA:
>>>      SET_NEXT_STATE (%.READY);
>>>      return 0;
>>>    case 0:
>>> -    length = be32toh (h->sbuf.reply.hdr.structured.length);
>>> +    length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */
>>>      offset = be64toh (h->sbuf.reply.payload.offset_data.offset);
>>>
>>>      assert (cmd); /* guaranteed by CHECK */
>>> @@ -346,7 +364,7 @@  REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA:
>>>      SET_NEXT_STATE (%.READY);
>>>      return 0;
>>>    case 0:
>>> -    length = be32toh (h->sbuf.reply.hdr.structured.length);
>>> +    length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */
>>>      offset = be64toh (h->sbuf.reply.payload.offset_data.offset);
>>>
>>>      assert (cmd); /* guaranteed by CHECK */
>>> @@ -426,7 +444,7 @@  REPLY.STRUCTURED_REPLY.RECV_BS_HEADER:
>>>      SET_NEXT_STATE (%.READY);
>>>      return 0;
>>>    case 0:
>>> -    length = be32toh (h->sbuf.reply.hdr.structured.length);
>>> +    length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */
>>>
>>>      assert (cmd); /* guaranteed by CHECK */
>>>      assert (cmd->type == NBD_CMD_BLOCK_STATUS);
>>> @@ -460,7 +478,7 @@  REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES:
>>>      SET_NEXT_STATE (%.READY);
>>>      return 0;
>>>    case 0:
>>> -    length = be32toh (h->sbuf.reply.hdr.structured.length);
>>> +    length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */
>>>
>>>      assert (cmd); /* guaranteed by CHECK */
>>>      assert (cmd->type == NBD_CMD_BLOCK_STATUS);
>>
>> (11) This is all fallout from (8). The commit message does document it
>> (in the first paragraph), but I don't understand where we *benefit* from
>> stuffing "nbd_extended_reply.length" into "nbd_structured_reply.length".
> 
> The benefit is that all later code only has to deal with
> nbd_structured_reply (rather than adding yet more 'if
> (h->extended_reply)' in all subsequent states).  Conversely, if I had
> widened instead of narrowed, then all later code would only need to
> doea with nbd_extended_reply.

Hmmm. That's a good point. I guess I didn't think of it because it would
mean (for example) handling extended block descriptors in a payload that
*seemingly* followed a structured (not extended) reply header.

I've not yet gotten used to the idea that header and payload are going
to be treated independently, after initial consistency checks.

> 
>>
>> Regarding downsides thereof, I can see two:
>>
>> - as I mentioned, "nbd_extended_reply.offset" becomes unusable,
>>
>> - "nbd_structured_reply.length" will no longer be big-endian but
>>   host-endian, which arguably does not match the other fields'
>>   endianness in the scratch space (= sbuf).
>>
>> I feel this back-stuffing brings the stashed header into an inconsistent
>> state that is specific to a subset of the automaton's states.
>>
>> I'd rather introduce an entirely new field to "sbuf.reply" -- between
>> "sbuf.reply.hdr" and "sbuf.reply.payload" --, if we really need to cache
>> a host-endian length value, regardless of which kind of reply header we
>> got.
> 
> You may be on to something.  Leaving the wire reply ALWAYS in network
> endian order, and adding a few bytes to nbd_reply to stash a
> normalized host-endian decoded value, would certainly be less
> confusing than having to figure out "which parts of my wire reply are
> still network-endian vs. natively converted".  It may make the overall
> structure a few bytes larger, but that's probably in the noise (since
> our nbd_handle already accomodatese maximum-length NBD strings of 4k,
> adding up to another 32 bytes for fully-normalized host-endian form
> isn't bad).  I'll keep that in mind for v4 (this is not the only place
> that needs normalization; handling 32- vs. 64-bit block status replies
> also needs normalization; the tradeoff between minimal storage by
> normalizing in place vs. maintainability by normalizing into a copy
> has interesting implications).
> 

Thanks!
Laszlo