Libguestfs - Sep 2023 - [PATCH v7 01/12] nbd/server: Support a request payload

On 27.09.23 18:59, Eric Blake wrote:
> We could also try to be a bit more complicated by peeking at the next
> few bytes: if they look like a magic number of the next request,
> assume the client set the bit accidentally but didn't send a payload
> after all; for anything else, assume the client did pass a payload.
> But adding in machinery to peek at a prefix is more complex than
> either assuming a payload is always present (as done in this patch) or
> assuming the bit was in error (and dropping the connection
> unconditionally).  Preferences?

Ohh, you are right, thanks for comprehensive explanation. I really missed some
things you are saying about. Yes, now I agree that "payload always exist
when flag is set" is the best effort. Finally, that was our aim of the
protocol design: make it more context independent. Probably, we may fix that in
specification as preferable or at least possible server behavior about
non-compliant client.

r-b coming soon, I just need to take another look with corrected picture in
mind.

-- 
Best regards,
Vladimir

On Thu, Sep 28, 2023 at 12:09:51PM +0300, Vladimir Sementsov-Ogievskiy
wrote:
> On 27.09.23 18:59, Eric Blake wrote:
> > We could also try to be a bit more complicated by peeking at the next
> > few bytes: if they look like a magic number of the next request,
> > assume the client set the bit accidentally but didn't send a
payload
> > after all; for anything else, assume the client did pass a payload.
> > But adding in machinery to peek at a prefix is more complex than
> > either assuming a payload is always present (as done in this patch) or
> > assuming the bit was in error (and dropping the connection
> > unconditionally).  Preferences?
> 
> 
> Ohh, you are right, thanks for comprehensive explanation. I really missed
some things you are saying about. Yes, now I agree that "payload always
exist when flag is set" is the best effort. Finally, that was our aim of
the protocol design: make it more context independent. Probably, we may fix that
in specification as preferable or at least possible server behavior about
non-compliant client.
One other possibility I just thought of: have a heuristic where the
flag set with h->request_length less than 512 bytes is likely to
indicate an intentional payload (even if for a command where we
weren't expecting payload, so still a client error); while the flag
set wtih h->request_length >= 512 bytes is likely to be a mistaken
setting of the flag (but also still a client error).  NBD_CMD_WRITE is
probably the only command that will ever need to send a payload larger
than one sector, but that command already has handling to accept
payloads of all sizes because we know what to do with them and where
the client is not in error.
> 
> r-b coming soon, I just need to take another look with corrected picture in
mind.
> 
> -- 
> Best regards,
> Vladimir
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org