Re: [Libguestfs] [PATCH v4 09/24] nbd: Replace bool structured_reply with mode enum
On Mon, Jun 12, 2023 at 02:24:52PM -0500, Eric Blake wrote:
> 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
> > > ---
> > >
> > > 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.
To move things along, I have now staged 1-8 in my NBD queue for a pull
request, and will then repost this patch and the remainder of the
series as v5, to make it easier to pick up the final needed R-b.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
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
> > ---
> >
> > 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", 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [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
---
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(, 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH v4 09/24] nbd: Replace bool structured_reply with mode enum
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
---
v4: new patch, expanding enum idea from v3 4/14
---
include/block/nbd.h | 2 +-
block/nbd.c | 8 +---
nbd/client-connection.c | 4 ++--
nbd/client.c| 18 +-
nbd/server.c| 27 +++
qemu-nbd.c | 4 +++-
6 files changed, 35 insertions(+), 28 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index aba4279b56c..fea69ac24bb 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -304,7 +304,7 @@ typedef struct NBDExportInfo {
/* In-out fields, set by client before nbd_receive_negotiate() and
* updated by server results during nbd_receive_negotiate() */
-bool structured_reply;
+NBDMode mode; /* input maximum mode tolerated; output actual mode chosen */
bool base_allocation; /* base:allocation context for NBD_CMD_BLOCK_STATUS
*/
/* Set by server results during nbd_receive_negotiate() and
diff --git a/block/nbd.c b/block/nbd.c
index 5322e66166c..5f88f7a819b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -464,7 +464,8 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState
*s, uint64_t cookie)
nbd_channel_error(s, ret);
return ret;
}
-if (nbd_reply_is_structured(>reply) && !s->info.structured_reply) {
+if (nbd_reply_is_structured(>reply) &&
+s->info.mode < NBD_MODE_STRUCTURED) {
nbd_channel_error(s, -EINVAL);
return -EINVAL;
}
@@ -867,7 +868,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
}
/* handle structured reply chunk */
-assert(s->info.structured_reply);
+assert(s->info.mode >= NBD_MODE_STRUCTURED);
chunk = >reply.structured;
if (chunk->type == NBD_REPLY_TYPE_NONE) {
@@ -1071,7 +1072,8 @@ nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t
cookie,
void *payload = NULL;
Error *local_err = NULL;
-NBD_FOREACH_REPLY_CHUNK(s, iter, cookie, s->info.structured_reply,
+NBD_FOREACH_REPLY_CHUNK(s, iter, cookie,
+s->info.mode >= NBD_MODE_STRUCTURED,
qiov, , )
{
int ret;
diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 3d14296c042..13e4cb6684b 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -1,5 +1,5 @@
/*
- * QEMU Block driver for NBD
+ * QEMU Block driver for NBD
*
* Copyright (c) 2021 Virtuozzo International GmbH.
*
@@ -93,7 +93,7 @@ NBDClientConnection *nbd_client_connection_new(const
SocketAddress *saddr,
.do_negotiation = do_negotiation,
.initial_info.request_sizes = true,
-.initial_info.structured_reply = true,
+.initial_info.mode = NBD_MODE_STRUCTURED,
.initial_info.base_allocation = true,
.initial_info.x_dirty_bitmap = g_strdup(x_dirty_bitmap),
.initial_info.name = g_strdup(export_name ?: "")
diff --git a/nbd/client.c b/nbd/client.c
index 479208d5d9d..faa054c4527 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -880,7 +880,7 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
static int nbd_start_negotiate(AioContext *aio_context, QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
const char *hostname, QIOChannel **outioc,
- bool structured_reply, bool *zeroes,
+ NBDMode max_mode, bool *zeroes,
Error **errp)
{
ERRP_GUARD();
@@ -958,7 +958,7 @@ static int nbd_start_negotiate(AioContext *aio_context,
QIOChannel *ioc,
if (fixedNewStyle) {
int result = 0;
-if (structured_reply) {
+if (max_mode >= NBD_MODE_STRUCTURED) {
result = nbd_request_simple_option(ioc,
NBD_OPT_STRUCTURED_REPLY,
false, errp);
@@ -1028,20 +1028,19 @@ int nbd_receive_negotiate(AioContext *aio_context,
QIOChannel *ioc,
trace_nbd_receive_negotiate_name(info->name);
result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc,
-
