Re: [Libguestfs] [PATCH v4 24/24] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS

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 <eblake(a)redhat.com&gt; > --- [..] > +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?