Re: [PATCH v7 12/12] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS

[Vladimir Sementsov-Ogievskiy]

On 25.09.23 22:22, Eric Blake wrote:
> Allow a client to request a subset of negotiated meta contexts.  For
> example, a client may ask to use a single connection to learn about
> both block status and dirty bitmaps, but where the dirty bitmap
> queries only need to be performed on a subset of the disk; forcing the
> server to compute that information on block status queries in the rest
> of the disk is wasted effort (both at the server, and on the amount of
> traffic sent over the wire to be parsed and ignored by the client).
>
> Qemu as an NBD client never requests to use more than one meta
> context, so it has no need to use block status payloads.  Testing this
> instead requires support from libnbd, which CAN access multiple meta
> contexts in parallel from a single NBD connection; an interop test
> submitted to the libnbd project at the same time as this patch
> demonstrates the feature working, as well as testing some corner cases
> (for example, when the payload length is longer than the export
> length), although other corner cases (like passing the same id
> duplicated) requires a protocol fuzzer because libnbd is not wired up
> to break the protocol that badly.
>
> This also includes tweaks to 'qemu-nbd --list' to show when a server
> is advertising the capability, and to the testsuite to reflect the
> addition to that output.
>
> Of note: qemu will always advertise the new feature bit during
> NBD_OPT_INFO if extended headers have alreay been negotiated
> (regardless of whether any NBD_OPT_SET_META_CONTEXT negotiation has
> occurred); but for NBD_OPT_GO, qemu only advertises the feature if
> block status is also enabled (that is, if the client does not
> negotiate any contexts, then NBD_CMD_BLOCK_STATUS cannot be used, so
> the feature is not advertised).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
> v5: factor out 'id - NBD_MTA_ID_DIRTY_BITMAP' [Vladimir], rework logic
> on zero-length requests to be clearer [Vladimir], rebase to earlier
> changes

[..]

> +/*
> + * nbd_co_block_status_payload_read
> + * Called when a client wants a subset of negotiated contexts via a
> + * BLOCK_STATUS payload.  Check the payload for valid length and
> + * contents.  On success, return 0 with request updated to effective
> + * length.  If request was invalid but all payload consumed, return 0
> + * with request->len and request->contexts->count set to 0 (which will
> + * trigger an appropriate NBD_EINVAL response later on).  Return
> + * negative errno if the payload was not fully consumed.
> + */
> +static int
> +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
> +                                 Error **errp)
> +{
> +    int payload_len = request->len;

payload_len should be uint64_t

> +    g_autofree char *buf = NULL;
> +    size_t count, i, nr_bitmaps;
> +    uint32_t id;
> +

otherwise, we may do something unexpected here, when reqeuest->len is too big 
for int:

> +    if (payload_len > NBD_MAX_BUFFER_SIZE) {
> +        error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)",
> +                   request->len, NBD_MAX_BUFFER_SIZE);
> +        return -EINVAL;
> +    }
> +
> +    assert(client->contexts.exp == client->exp);
> +    nr_bitmaps = client->exp->nr_export_bitmaps;
> +    request->contexts = g_new0(NBDMetaContexts, 1);
> +    request->contexts->exp = client->exp;
> +
> +    if (payload_len % sizeof(uint32_t) ||
> +        payload_len < sizeof(NBDBlockStatusPayload) ||
> +        payload_len > (sizeof(NBDBlockStatusPayload) +
> +                       sizeof(id) * client->contexts.count)) {
> +        goto skip;
> +    }

[..]

>    * connection right away, -EAGAIN to indicate we were interrupted and the
> @@ -2505,7 +2593,18 @@ static int coroutine_fn 
> nbd_co_receive_request(NBDRequestData *req,
>           break;
>
>       case NBD_CMD_BLOCK_STATUS:
> -        request->contexts = &client->contexts;
> +        if (extended_with_payload) {
> +            ret = nbd_co_block_status_payload_read(client, request, errp);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +            /* payload now consumed */
> +            check_length = extended_with_payload = false;

why set extended_with_payload to false? it's a bit misleading. And you don't do 
this for WRITE request.

> +            payload_len = 0;
> +            valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> +        } else {
> +            request->contexts = &client->contexts;
> +        }
>           valid_flags |= NBD_CMD_FLAG_REQ_ONE;
>           break;

[..]


with payload_len changed to uint64_t, your squash-in applied and s/>/>=/ fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>


--
Best regards,
Vladimir