8:14 a.m.
On Fri, Jun 02, 2023 at 12:13:25PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On 15.05.23 22:53, 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).
>
...
>
> +/*
> + * 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 payload consumed, return 0 with
> + * request->len and request->contexts.count set to 0 (which will
> + * trigger an appropriate NBD_EINVAL response later on).
Hmm. So, it leads to
case NBD_CMD_BLOCK_STATUS:
if (!request->len) {
return nbd_send_generic_reply(client, request, -EINVAL,
"need non-zero length", errp);
}
EINVAL is OK, but "need non-zero length" message is not appropriate.. I think
we need separate reply for the case of invalid payload.
Or maybe just reword the error to "unexpected length", which covers a
broader swath of errors (none of which are likely from compliant
clients).
> On I/O
> + * error, return -EIO.
> + */
> +static int
> +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
> + Error **errp)
> +{
> + int payload_len = request->len;
> + g_autofree char *buf = NULL;
> + g_autofree bool *bitmaps = NULL;
> + size_t count, i;
> + uint32_t id;
> +
> + assert(request->len <= NBD_MAX_BUFFER_SIZE);
> + if (payload_len % sizeof(uint32_t) ||
> + payload_len < sizeof(NBDBlockStatusPayload) ||
> + payload_len > (sizeof(NBDBlockStatusPayload) +
> + sizeof(id) * client->contexts.count)) {
> + goto skip;
> + }
> +
> + buf = g_malloc(payload_len);
> + if (nbd_read(client->ioc, buf, payload_len,
> + "CMD_BLOCK_STATUS data", errp) < 0) {
> + return -EIO;
> + }
> + trace_nbd_co_receive_request_payload_received(request->handle,
> + payload_len);
> + memset(&request->contexts, 0, sizeof(request->contexts));
> + request->contexts.nr_bitmaps =
client->context_exp->nr_export_bitmaps;
> + bitmaps = g_new0(bool, request->contexts.nr_bitmaps);
> + count = (payload_len - sizeof(NBDBlockStatusPayload)) / sizeof(id);
In doc we have MUST: "The payload form MUST occupy 8 + n*4 bytes", do we really
want to forgive and ignore unaligned tail? May be better to "goto skip" in this
case, to avoid ambiguity.
That's what happened above, when checking that payload_len %
sizeof(uint32_t) was 0. Or am I misunderstanding your question about
another condition where goto skip would be appropriate?
> + payload_len = 0;
> +
> + for (i = 0; i < count; i++) {
> +
> + id = ldl_be_p(buf + sizeof(NBDBlockStatusPayload) + sizeof(id) * i);
> + if (id == NBD_META_ID_BASE_ALLOCATION) {
> + if (request->contexts.base_allocation) {
> + goto skip;
> + }
> + request->contexts.base_allocation = true;
> + } else if (id == NBD_META_ID_ALLOCATION_DEPTH) {
> + if (request->contexts.allocation_depth) {
> + goto skip;
> + }
> + request->contexts.allocation_depth = true;
> + } else {
> + if (id - NBD_META_ID_DIRTY_BITMAP >
> + request->contexts.nr_bitmaps ||
> + bitmaps[id - NBD_META_ID_DIRTY_BITMAP]) {
> + goto skip;
> + }
> + bitmaps[id - NBD_META_ID_DIRTY_BITMAP] = true;
> + }
> + }
> +
> + request->len = ldq_be_p(buf);
> + request->contexts.count = count;
> + request->contexts.bitmaps = bitmaps;
> + bitmaps = NULL;
better I think:
request->context.bitmaps = g_steal_pointer(bitmaps);
- as a pair to g_autofree.
Yes, that is shorter.
> + return 0;
> +
> + skip:
> + trace_nbd_co_receive_block_status_payload_compliance(request->from,
> + request->len);
> + request->len = request->contexts.count = 0;
> + return nbd_drop(client->ioc, payload_len, errp);
> +}
> +
> /* nbd_co_receive_request
> * Collect a client request. Return 0 if request looks valid, -EIO to drop
> * connection right away, -EAGAIN to indicate we were interrupted and the
> @@ -2461,7 +2547,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData
*req, NBDRequest *
>
> if (request->type == NBD_CMD_WRITE || extended_with_payload) {
> payload_len = request->len;
> - if (request->type != NBD_CMD_WRITE) {
> + if (request->type == NBD_CMD_BLOCK_STATUS) {
> + payload_len = nbd_co_block_status_payload_read(client,
> + request,
> + errp);
> + if (payload_len < 0) {
> + return -EIO;
> + }
Seems we can handle all payload in one "switch" block, instead of handling
BLOCK_STATUS here and postponing WRITE payload (and dropping) up to the end of the block
with help of payload_len variable.
I can experiment with other ways of respresenting the logic; there's
enough complexity that I'm trying to balance between repeating
conditionals vs. avoiding added nesting.
> + } else if (request->type != NBD_CMD_WRITE) {
> /*
> * For now, we don't support payloads on other
> * commands; but we can keep the connection alive.
> @@ -2540,6 +2633,9 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData
*req, NBDRequest *
> valid_flags |= NBD_CMD_FLAG_NO_HOLE | NBD_CMD_FLAG_FAST_ZERO;
> } else if (request->type == NBD_CMD_BLOCK_STATUS) {
> valid_flags |= NBD_CMD_FLAG_REQ_ONE;
> + if (client->extended_headers && client->contexts.count) {
> + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> + }
> }
> if (request->flags & ~valid_flags) {
> error_setg(errp, "unsupported flags for command %s (got 0x%x)",
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org