Re: Structured replies in nbd-server

[Wouter Verhelst <w@uter.be>]

So, to provide some feedback:

On Thu, Mar 09, 2023 at 03:19:13PM -0600, Eric Blake wrote:
> On Tue, Mar 07, 2023 at 07:20:16PM +0200, Wouter Verhelst wrote:
> > Hi,
> > 
> > I've finally been working on structured replies in nbd-server (what?
> > it's only been 7 years), and today I managed to make it go through
> > nbdkit's "nbddump" without crashing. (yay!)
> 
> 'nbddump' comes from the libnbd project (client side), not nbdkit
> (server side).  But the two are closely related ;) And I'm glad to see
> that it helped you!
> 
> > 
> > I'm currently not (yet) implementing any other features that depend on
> > structured replies, but would like to see if people can poke holes in
> > what I've done so far -- I'd like to find bugs before release, rather
> > than after ;-)
> > 
> > The code is in the "structured" branch of
> > https://github.com/NetworkBlockDevice/nbd and feedback is more than
> > welcome.
> 
> Since you didn't post the code to the list, I've likewise only left my
> review comments on the patches on github.  If we want to copy the
> discussion to the list, I can do that too.

Some of your comments were:

>  I don't know if it is worth trying to piggyback
>  NBD_REPLY_FLAG_DONE to the last data or error chunk instead for less
>  network traffic, but the spec (intentionally) does not mandate that
>  optimization.

Yeah, I considered that but decided against it in the end. The extra
bookkeeping that would be required to be able to implement that felt
like too much effort to me.

(I did notice that I forgot to send the DONE flag in the case the client
sent the DF flag, which seems like a massive error -- will need to fix
that)

>  Maybe word the comment as "standard requires a minimum of 64KiB; we are
>  more generous by allowing up to 1MiB as our largest unfragmented
>  answer"

Thanks, that does sound better. Modified as such.

>  BUG - you must ALSO do socket_read() of len bytes, or disconnect
>  (particularly if len is larger than any NBD_OPT_ request you are
>  willing to accept) - otherwise, you are now out of sync with the
>  client's next request.

Thanks, good catch. Will be fixed for v2.

>  Reuse consume_len() for this purpose, rather than open-coding it. At
>  which point, do you really need buf[1024], or can you just directly
>  socket_read(&len)?

consume_len() is implemented in terms of consume(), but only accepts a
CLIENT* parameter, and expects to read the length from the socket first.
We've already done that at this point, so we can't. However, we can call
consume(), which does need a buffer (so that means we'll have keep it
then).

>  Possible improvement: if F_STRUCTURED is already set in
>  client->clientflags, the spec allows but does not require you to reply
>  NBD_REP_EINVAL to point out the client's redundant request.

That is indeed simple enough to implement, so added that, thanks.

>  BUG - this won't compile when TLS is linked in (clientflags, not
>  clientfeats). But you are correct that you must remove F_STRUCTURED
>  from client->clientflags during handling of NBD_OPT_STARTTLS, and blind
>  assignment to 0 also wipes out any other features we may add over time.

Indeed, I wiped out the wrong variable :-D

Thanks, good catch. Fixed for v2.

-- 
     w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}

I will have a Tin-Actinium-Potassium mixture, thanks.