On Tue, Jun 27, 2023 at 10:42:20PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.06.23 16:56, 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.
> >
> > Signed-off-by: Eric Blake <ebl...@redhat.com>
> > ---
>
> [..]
>
> > +static int
> > +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
> > + Error **errp)
> > +{
> > + int payload_len = request->len;
> > + g_autofree char *buf = NULL;
> > + size_t count, i, nr_bitmaps;
> > + uint32_t id;
> > +
> > + assert(request->len <= NBD_MAX_BUFFER_SIZE);
> > + 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;
> > + }
> > @@ -2470,7 +2552,13 @@ static int coroutine_fn
> > nbd_co_receive_request(NBDRequestData *req, NBDRequest *
>
> [..]
>
> > @@ -2712,7 +2804,8 @@ static coroutine_fn int nbd_handle_request(NBDClient
> > *client,
> > "discard failed", errp);
> >
> > case NBD_CMD_BLOCK_STATUS:
> > - if (!request->len) {
> > + assert(request->contexts);
> > + if (!request->len && !(request->flags & NBD_CMD_FLAG_PAYLOAD_LEN))
> > {
>
> why not error-out for len=0 in case of payload?
Because nbd_co_block_status_payload_read() already rejected that case.
But an assertion would not hurt to make it obvious.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
