On Wed, Feb 21, 2024 at 02:22:36PM -0600, Eric Blake wrote:
On Wed, Feb 21, 2024 at 07:08:05PM +0000, Richard W.M. Jones wrote:
On Wed, Feb 21, 2024 at 08:53:51AM -0600, Eric Blake wrote:
Commit 55b09667 in v1.15.3 made it possible for nbd_can_meta_context() to return an error if it is called too early during handshaking, which is useful when doing manual control via nbd_set_opt_mode for integration testing. But it had the unfortunate side effect of treating ALL server errors as unknown state for meta contexts. This didn't matter for clients that never attempt meta context negotiation, or for servers that don't support structured replies (nbd-server 3.24); but for servers with structured replies but not meta contexts (nbd-server 3.25), it means we are now reporting an error about failure to negotiate rather than silently treating the query as false. And with this patch in place, the one-off hack to nbdkit mentioned in that commit (when a server fails after partial success, rather than immediately) still has the desired effect of an error in nbd_can_meta_context.
The test for this fix is in a separate commit, to make it easier to rearrange the patch series to demonstrate that the issue at hand can also be worked around in client code.
Fiexs: 55b09667 ("api: Fix nbd_can_meta_context if NBD_OPT_SET_META_CONTEXT fails", v1.15.3)
s/Fiexs/Fixes/
D'oh; fixed locally.
Signed-off-by: Eric Blake <eblake@redhat.com> --- generator/states-newstyle-opt-meta-context.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index 47e0fcd5..b62a7df0 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -281,6 +281,8 @@ NEWSTYLE.OPT_META_CONTEXT.CHECK_REPLY: SET_NEXT_STATE (%.NEGOTIATING); } else { + if (opt == NBD_OPT_SET_META_CONTEXT && h->meta_contexts.len == 0) + h->meta_valid = true;
I'm not sure I understand this. Shouldn't it be setting this to false?
The act of starting a context negotiation sets h->meta_valid = false with meta_contexts.len == 0. If the server replies with NBD_REP_ACK (no matter if there were 0, 1, or many intervening NBD_REP_META_CONTEXT), we then set h->meta_valid = true since the server reported success at the end of a list (that code was already in place at line 224). But this section of code is when the server replies with NBD_REP_ERR_*; here, we want to treat any immediate error (with no intervening NBD_REP_META_CONTEXT) as a successful empty list (we know that the server doesn't support any metacontexts), but any late error (with one or more intervening NBD_REP_META_CONTEXT) should leave things to the already-set meta_valid=false so that we aren't confusing ourselves on a partial list before a late error.
Would it make more sense if I wrote it as:
/* When ignoring error, treat attempt as success only if list is empty */ if (opt == NBD_OPT_SET_META_CONTEXT) h->meta_valid = h->meta_contexts.len == 0;
That's nice, thanks! Reviewed-by: Richard W.M. Jones <rjones@redhat.com> -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top