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

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 at 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
> @@ -143,7 +143,7 @@ struct NBDClient {
> 
>       uint32_t check_align; /* If non-zero, check for aligned client
requests */
> 
> -    bool structured_reply;
> +    NBDMode mode;
>       NBDExportMetaContexts export_meta;
> 
>       uint32_t opt; /* Current option being negotiated */
> @@ -502,7 +502,7 @@ static int nbd_negotiate_handle_export_name(NBDClient
*client, bool no_zeroes,
>       }
> 
>       myflags = client->exp->nbdflags;
> -    if (client->structured_reply) {
> +    if (client->mode >= NBD_MODE_STRUCTURED) {
>           myflags |= NBD_FLAG_SEND_DF;
>       }
>       trace_nbd_negotiate_new_style_size_flags(client->exp->size,
myflags);
> @@ -687,7 +687,7 @@ static int nbd_negotiate_handle_info(NBDClient *client,
Error **errp)
> 
>       /* Send NBD_INFO_EXPORT always */
>       myflags = exp->nbdflags;
> -    if (client->structured_reply) {
> +    if (client->mode >= NBD_MODE_STRUCTURED) {
>           myflags |= NBD_FLAG_SEND_DF;
>       }
>       trace_nbd_negotiate_new_style_size_flags(exp->size, myflags);
> @@ -985,7 +985,8 @@ static int nbd_negotiate_meta_queries(NBDClient
*client,
>       size_t i;
>       size_t count = 0;
> 
> -    if (client->opt == NBD_OPT_SET_META_CONTEXT &&
!client->structured_reply) {
> +    if (client->opt == NBD_OPT_SET_META_CONTEXT &&
> +        client->mode < NBD_MODE_STRUCTURED) {
>           return nbd_opt_invalid(client, errp,
>                                  "request option '%s' when
structured reply "
>                                  "is not negotiated",
> @@ -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.
>                   }
>                   break;
> 
> @@ -1907,7 +1908,9 @@ static int coroutine_fn
nbd_co_send_simple_reply(NBDClient *client,
>       };
> 
>       assert(!len || !nbd_err);
> -    assert(!client->structured_reply || request->type !=
NBD_CMD_READ);
> +    assert(client->mode < NBD_MODE_STRUCTURED ||
> +           (client->mode == NBD_MODE_STRUCTURED &&
> +            request->type != NBD_CMD_READ));
>       trace_nbd_co_send_simple_reply(request->cookie, nbd_err,
>                                      nbd_err_lookup(nbd_err), len);
>       set_be_simple_reply(&reply, nbd_err, request->cookie);
> @@ -2409,7 +2412,7 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req, NBDRequest *
>                                                 client->check_align);
>       }
>       valid_flags = NBD_CMD_FLAG_FUA;
[..]

-- 
Best regards,
Vladimir

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 at 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