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