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

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(a)redhat.com&gt; > > --- > > > > 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.