[Libguestfs] Re: [libnbd PATCH 2/3] api: Fix nbd_can_meta_context for server that lacks meta contexts
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(a)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;
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org