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 <ebl...@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
