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