Re: [Libguestfs] [PATCH v7 12/12] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS

[Eric Blake]

On Wed, Oct 04, 2023 at 04:55:02PM -0500, Eric Blake wrote:
> > > +static int
> > > +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
> > > +                                 Error **errp)
> > 
> > [..]

> > > +    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;
> > > +            }
> > 
> > should we also check that base_allocation is negotiated?
> 
> Oh, good point.  Without that check, the client can pass in random id
> numbers that it never negotiated.  I've queued 1-11 and will probably
> send a pull request for those this week, while respinning this patch
> to fix the remaining issues you pointed out.

I'm squashing in the following. If you can review it today, I'll
include it in my pull request this afternoon; if not, we still have
time before soft freeze to get it in the next batch.

diff --git i/nbd/server.c w/nbd/server.c
index 30816b42386..62654579cbc 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -2478,19 +2478,22 @@ nbd_co_block_status_payload_read(NBDClient *client, 
NBDRequest *request,
     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) {
+            if (!client->contexts.base_allocation ||
+                request->contexts->base_allocation) {
                 goto skip;
             }
             request->contexts->base_allocation = true;
         } else if (id == NBD_META_ID_ALLOCATION_DEPTH) {
-            if (request->contexts->allocation_depth) {
+            if (!client->contexts.allocation_depth ||
+                request->contexts->allocation_depth) {
                 goto skip;
             }
             request->contexts->allocation_depth = true;
         } else {
-            int idx = id - NBD_META_ID_DIRTY_BITMAP;
+            unsigned idx = id - NBD_META_ID_DIRTY_BITMAP;

-            if (idx > nr_bitmaps || request->contexts->bitmaps[idx]) {
+            if (idx > nr_bitmaps || !client->contexts.bitmaps[idx] ||
+                request->contexts->bitmaps[idx]) {
                 goto skip;
             }
             request->contexts->bitmaps[idx] = true;
diff --git i/nbd/trace-events w/nbd/trace-events
index 3cf2d00e458..00ae3216a11 100644
--- i/nbd/trace-events
+++ w/nbd/trace-events
@@ -70,7 +70,7 @@ nbd_co_send_chunk_read(uint64_t cookie, uint64_t offset, void 
*data, uint64_t si
 nbd_co_send_chunk_read_hole(uint64_t cookie, uint64_t offset, uint64_t size) 
"Send structured read hole reply: cookie = %" PRIu64 ", offset = %" PRIu64 ", 
len = %" PRIu64
 nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, 
uint64_t length, int last) "Send block status reply: cookie = %" PRIu64 ", 
extents = %u, context = %d (extents cover %" PRIu64 " bytes, last chunk = %d)"
 nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const 
char *msg) "Send structured error reply: cookie = %" PRIu64 ", error = %d (%s), 
msg = '%s'"
-nbd_co_receive_block_status_payload_compliance(uint64_t from, int len) "client 
sent unusable block status payload: from=0x%" PRIx64 ", len=0x%x"
+nbd_co_receive_block_status_payload_compliance(uint64_t from, uint64_t len) 
"client sent unusable block status payload: from=0x%" PRIx64 ", len=0x%" PRIx64
 nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char 
*name) "Decoding type: cookie = %" PRIu64 ", type = %" PRIu16 " (%s)"
 nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len) 
"Payload received: cookie = %" PRIu64 ", len = %" PRIu64
 nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client 
sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" 
PRIx64



-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org