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
