Re: [Libguestfs] [PATCH v3 14/14] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS

[Eric Blake]

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