[Libguestfs] [libnbd PATCH v4 23/25] api: Add nbd_can_block_status_payload()

[Eric Blake]

On Wed, Aug 02, 2023 at 08:50:43PM -0500, Eric Blake wrote:
> In the recent NBD protocol extensions to add 64-bit commands [1], an
> additional option was added to allow NBD_CMD_BLOCK_STATUS to pass a
> client payload instructing the server to filter its answers in nbd.git
> commit e6f3b94a (mainly useful when the client requests more than one
> meta context with NBD_OPT_SET_META_CONTEXT).  This patch lays the
> groundwork by exposing servers that advertise this capability,
> although libnbd does not yet actually utilize it until the next patch.
> 
> At the time this patch was written, qemu-nbd was also patched to
> provide such support; hence, an interop/ test shows the API in action.
> 
> [1] https://github.com/NetworkBlockDevice/nbd/blob/extension-ext-header/doc/
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
> ---
> 
> v4: rebase to earlier changes, s/can/has/ [Rich]
> ---

This one has been giving me some grief in my integration testing.  It
turns out that my qemu patches chose the following logic: if
requesting NBD_OPT_INFO, advertise the block_status_payload feature
unconditionally (regardless of whether NBD_OPT_SET_META_CONTEXT has
been called); but when requesting NBD_OPT_GO, only advertise the
block_status_payload feature when it is actually usable (that is, when
the caller negotiated at least one metacontext).

The upshot of that: 'nbdinfo -- [ qemu-nbd ... ] | grep payload' shows
can_block_status_payload:true (because that was determined by
NBD_OPT_INFO), but 'nbd --can block-status-payload -- [ qemu-nbd
... ]' shows false (because we skip straight to NBD_OPT_GO without
requesting any meta contexts).  I'll have to add in another patch to
nbdinfo to prefer opt mode by default.

> +++ b/info/nbdinfo.pod
> @@ -171,6 +171,11 @@ for supporting block status commands).
>  Test if server supports extended headers (a prerequisite for
>  supporting 64-bit commands; implies structured replies as well).
> 
> +=item nbdinfo --has block-status-payload URI
> +
> +Test if server has support for passing a client payload to limit the
> +response to a block status command.
> +
>  =item nbdinfo --is rotational URI
> 
>  Test if the server export is backed by something which behaves like a
> @@ -373,6 +378,8 @@ When using I<--list>, the default is I<--no-content> (since
>  downloading from each export is expensive).  To enable content probing
>  use I<--list --content>.
> 
> +=item B<--has block-status-payload>
> +
>  =item B<--has extended-headers>
> 
>  =item B<--has structured-reply>
> @@ -385,6 +392,7 @@ indicate an error querying the flag).
>  For further information see the L<NBD
>  protocol|https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md>
>  and the following libnbd functions:
> +L<nbd_can_block_status_payload(3)>,
>  L<nbd_get_extended_headers_negotiated(3)>,
>  L<nbd_get_structured_replies_negotiated(3)>.

I'm second-guessing this naming.  While '--can extended-headers' does
not match any API name, and '--has extended-headers' reads nicely and
corresponds to the fact that the condition is a property of the server
as a whole (regardless of which export you connect to), '--can
block-status-payload' better matches the API call as well as the fact
that it is a per-export decision (in the qemu case, the feature is not
present if you did not specifically negotiate meta-contexts for the
export in question, at least for NBD_OPT_GO).  Both spellings will
still work, but I'll probably change this patch to stick to the --can
naming from v3 (while leaving the --has naming for extended-headers)
as the documented canonical spelling.

> +++ b/interop/block-status-payload.sh
...
> +
> +# Conditional part of test: if qemu is new enough to support extended
> +# headers, we assume it can also support block status payload.
> +requires nbdinfo --can extended-headers -- [ qemu-nbd -r -f qcow2 "$file" ]
> +$VG ./block-status-payload \
> +    qemu-nbd -f qcow2 -A -B bitmap0 -B bitmap1 $file

This part is where I hit the integration failures.  Just because qemu
supports extended headers does not mean it supports block status
payload (at least, not if you bisect my qemu patch series at the right
point in time), even if it is true for a released version of qemu.
But as I just found out, testing for --can block-status-payload is
trickier unless I first teach nbdinfo to prefer NBD_OPT_INFO probing,
even when not producing full output.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org