Re: [Libguestfs] [PATCH v4 09/24] nbd: Replace bool structured_reply with mode enum

[Eric Blake]

On Mon, Jun 12, 2023 at 06:07:59PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.06.23 16:56, Eric Blake wrote:
> > The upcoming patches for 64-bit extensions requires various points in
> > the protocol to make decisions based on what was negotiated.  While we
> > could easily add a 'bool extended_headers' alongside the existing
> > 'bool structured_reply', this does not scale well if more modes are
> > added in the future.  Better is to expose the mode enum added in the
> > previous patch out to a wider use in the code base.
> > 
> > Where the code previously checked for structured_reply being set or
> > clear, it now prefers checking for an inequality; this works because
> > the nodes are in a continuum of increasing abilities, and allows us to
> > touch fewer places if we ever insert other modes in the middle of the
> > enum.  There should be no semantic change in this patch.
> > 
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> > 
> > v4: new patch, expanding enum idea from v3 4/14
> > ---
> 
> [..]
> 
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 8486b64b15d..bade4f7990c 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c

> > @@ -1261,13 +1262,13 @@ static int nbd_negotiate_options(NBDClient *client, 
> > Error **errp)
> >               case NBD_OPT_STRUCTURED_REPLY:
> >                   if (length) {
> >                       ret = nbd_reject_length(client, false, errp);
> > -                } else if (client->structured_reply) {
> > +                } else if (client->mode >= NBD_MODE_STRUCTURED) {
> >                       ret = nbd_negotiate_send_rep_err(
> >                           client, NBD_REP_ERR_INVALID, errp,
> >                           "structured reply already negotiated");
> >                   } else {
> >                       ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, 
> > errp);
> > -                    client->structured_reply = true;
> > +                    client->mode = NBD_MODE_STRUCTURED;
> 
> Hmm. in all other cases in server code client.mode remains zero = OLDSTYLE, 
> which is not quite correct.

Good catch.  Consider this squashed in (note that as a server we NEVER
talk NBD_MODE_OLDSTYLE - we ripped that out back in commit 7f7dfe2a;
but whether we end up on EXPORT_NAME or SIMPLE depends on the client's
response to our initial flag advertisement.  The only reason I didn't
spot it sooner is that in the server, all subsequent checks of
client->mode grouped OLDSTYLE, EXPORT_NAME, and SIMPLE into the same
handling.

diff --git a/nbd/server.c b/nbd/server.c
index bade4f7990c..bc6858cafe6 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1123,10 +1123,12 @@ static int nbd_negotiate_options(NBDClient *client, 
Error **errp)
     if (nbd_read32(client->ioc, &flags, "flags", errp) < 0) {
         return -EIO;
     }
+    client->mode = NBD_MODE_EXPORT_NAME;
     trace_nbd_negotiate_options_flags(flags);
     if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
         fixedNewstyle = true;
         flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
+        client->mode = NBD_MODE_SIMPLE;
     }
     if (flags & NBD_FLAG_C_NO_ZEROES) {
         no_zeroes = true;


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org