Re: [PATCH v4 20/24] nbd/client: Accept 64-bit block status chunks

[Vladimir Sementsov-Ogievskiy]

On 08.06.23 16:56, Eric Blake wrote:
> Once extended mode is enabled, we need to accept 64-bit status replies
> (even for replies that don't exceed a 32-bit length).  It is easier to
> normalize narrow replies into wide format so that the rest of our code
> only has to handle one width.  Although a server is non-compliant if
> it sends a 64-bit reply in compact mode, or a 32-bit reply in extended
> mode, it is still easy enough to tolerate these mismatches.
>
> In normal execution, we are only requesting "base:allocation" which
> never exceeds 32 bits for flag values. But during testing with
> x-dirty-bitmap, we can force qemu to connect to some other context
> that might have 64-bit status bit; however, we ignore those upper bits
> (other than mapping qemu:allocation-depth into something that
> 'qemu-img map --output=json' can expose), and since that only affects
> testing, we really don't bother with checking whether more than the
> two least-significant bits are set.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

[..]

> +     * count at 0 for a narrow server).  However, it's easy enough to
> +     * ignore the server's noncompliance without killing the
>        * connection; just ignore trailing extents, and clamp things to
>        * the length of our request.
>        */
> -    if (chunk->length > sizeof(context_id) + sizeof(*extent)) {
> -        trace_nbd_parse_blockstatus_compliance("more than one extent");
> +    if (count != wide ||
> +        chunk->length > sizeof(context_id) + wide * sizeof(count) + len) {

this calculation is done twice. I'd put it into expected_chunk_length variable.

> +        trace_nbd_parse_blockstatus_compliance("unexpected extent count");
>       }
>       if (extent->length > orig_length) {
>           extent->length = orig_length;
> @@ -1123,7 +1136,7 @@ nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t 
> cookie,
>
>   static int coroutine_fn
>   nbd_co_receive_blockstatus_reply(BDRVNBDState *s, uint64_t cookie,

[..]

--
Best regards,
Vladimir