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

On 5/25/23 15:00, Eric Blake wrote: (1) We might want to replace "handle" with "cookie" at this point. Ah, yes. If we negotiate extended headers, we set "rlen" to "sizeof extended" in REPLY.START, which is larger than either of "sizeof simple" and "sizeof structured". Therefore we'll get stuck in REPLY.RECV_REPLY. Correct? Not liking this "not worth it" argument. But... ... that sounds OK! Probably best to reorder the files in this patch for review: Seems OK. 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"? Heh, I love this ((a == b) == c) "equivalence" form! :) 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): /--------------------- | diff --git a/generator/states-reply.c b/generator/states-reply.c | index 4e9f2ddeb567..ea7bd4e7db91 100644 | --- a/generator/states-reply.c | +++ b/generator/states-reply.c | @@ -133,15 +133,8 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY: | SET_NEXT_STATE (%STRUCTURED_REPLY.START); | 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), | - stderr); | -#endif | - return 0; | + /* We've probably lost synchronization. */ | + goto invalid: | } | | /* NB: This works for both simple and structured replies because the | @@ -159,6 +152,16 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY: | h->reply_cmd = cmd; | return 0; | | +invalid: | + SET_NEXT_STATE (%.DEAD); | + 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), | + stderr); | +#endif | + return 0; | + | REPLY.FINISH_COMMAND: | struct command *prev_cmd, *cmd; | uint64_t cookie; \--------------------- ... 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.) Then: (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". 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 { | 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, - 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 Then: (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. (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. (8) I'm very confused by this. For an extended reply, this will overwrite the "offset" field. I understand we have stolen that field already, above; but that's little solace, especially without specific comments. (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. (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. 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.) (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". 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. Thanks! Laszlo