| Message ID | 20230608135653.2918540-24-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu patches for 64-bit NBD extensions | expand |
On Thu, Jun 08, 2023 at 08:56:52AM -0500, Eric Blake wrote: > The next commit will add support for the optional extension > NBD_CMD_FLAG_PAYLOAD during NBD_CMD_BLOCK_STATUS, where the client can > request that the server only return a subset of negotiated contexts, > rather than all contexts. To make that task easier, this patch > populates the list of contexts to return on a per-command basis (for > now, identical to the full set of negotiated contexts). > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > +++ b/nbd/server.c > @@ -2491,6 +2491,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * > error_setg(errp, "No memory"); > return -ENOMEM; > } > + } else if (request->type == NBD_CMD_BLOCK_STATUS) { > + request->contexts = &client->contexts; > } THere are paths where request->contexts is left NULL but request->type was set... > @@ -2841,6 +2848,11 @@ static coroutine_fn void nbd_trip(void *opaque) > } else { > ret = nbd_handle_request(client, &request, req->data, &local_err); > } > + if (request.type == NBD_CMD_BLOCK_STATUS && > + request.contexts != &client->contexts) { > + g_free(request.contexts->bitmaps); > + g_free(request.contexts); > + } so I think this also needs to be tweaked to check that request.contexts is non-NULL before dereferencing to free bitmaps.
On 08.06.23 22:15, Eric Blake wrote: > On Thu, Jun 08, 2023 at 08:56:52AM -0500, Eric Blake wrote: >> The next commit will add support for the optional extension >> NBD_CMD_FLAG_PAYLOAD during NBD_CMD_BLOCK_STATUS, where the client can >> request that the server only return a subset of negotiated contexts, >> rather than all contexts. To make that task easier, this patch >> populates the list of contexts to return on a per-command basis (for >> now, identical to the full set of negotiated contexts). >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> > >> +++ b/nbd/server.c >> @@ -2491,6 +2491,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * >> error_setg(errp, "No memory"); >> return -ENOMEM; >> } >> + } else if (request->type == NBD_CMD_BLOCK_STATUS) { >> + request->contexts = &client->contexts; >> } > > THere are paths where request->contexts is left NULL but request->type > was set... > >> @@ -2841,6 +2848,11 @@ static coroutine_fn void nbd_trip(void *opaque) >> } else { >> ret = nbd_handle_request(client, &request, req->data, &local_err); >> } >> + if (request.type == NBD_CMD_BLOCK_STATUS && >> + request.contexts != &client->contexts) { >> + g_free(request.contexts->bitmaps); >> + g_free(request.contexts); >> + } > > so I think this also needs to be tweaked to check that > request.contexts is non-NULL before dereferencing to free bitmaps. > agree (and I missed this during my review :) suggest: if (request.contexts && request.contexts != &client->contexts) { g_free(..) g_free(..) }
diff --git a/include/block/nbd.h b/include/block/nbd.h index f240707f646..47850be5a66 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -76,6 +76,7 @@ typedef struct NBDRequest { uint16_t flags; /* NBD_CMD_FLAG_* */ uint16_t type; /* NBD_CMD_* */ NBDMode mode; /* Determines which network representation to use */ + NBDMetaContexts *contexts; /* Used by NBD_CMD_BLOCK_STATUS */ } NBDRequest; typedef struct NBDSimpleReply { diff --git a/nbd/server.c b/nbd/server.c index 42a4300c95e..308846fe46b 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2491,6 +2491,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * error_setg(errp, "No memory"); return -ENOMEM; } + } else if (request->type == NBD_CMD_BLOCK_STATUS) { + request->contexts = &client->contexts; } if (payload_len) { @@ -2716,11 +2718,11 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, } assert(client->mode >= NBD_MODE_EXTENDED || request->len <= UINT32_MAX); - if (client->contexts.count) { + if (request->contexts->count) { bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE; - int contexts_remaining = client->contexts.count; + int contexts_remaining = request->contexts->count; - if (client->contexts.base_allocation) { + if (request->contexts->base_allocation) { ret = nbd_co_send_block_status(client, request, exp->common.blk, request->from, @@ -2733,7 +2735,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, } } - if (client->contexts.allocation_depth) { + if (request->contexts->allocation_depth) { ret = nbd_co_send_block_status(client, request, exp->common.blk, request->from, request->len, @@ -2746,8 +2748,9 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, } } + assert(request->contexts->exp == client->exp); for (i = 0; i < client->exp->nr_export_bitmaps; i++) { - if (!client->contexts.bitmaps[i]) { + if (!request->contexts->bitmaps[i]) { continue; } ret = nbd_co_send_bitmap(client, request, @@ -2763,6 +2766,10 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, assert(!contexts_remaining); return 0; + } else if (client->contexts.count) { + return nbd_send_generic_reply(client, request, -EINVAL, + "CMD_BLOCK_STATUS payload not valid", + errp); } else { return nbd_send_generic_reply(client, request, -EINVAL, "CMD_BLOCK_STATUS not negotiated", @@ -2841,6 +2848,11 @@ static coroutine_fn void nbd_trip(void *opaque) } else { ret = nbd_handle_request(client, &request, req->data, &local_err); } + if (request.type == NBD_CMD_BLOCK_STATUS && + request.contexts != &client->contexts) { + g_free(request.contexts->bitmaps); + g_free(request.contexts); + } if (ret < 0) { error_prepend(&local_err, "Failed to send reply: "); goto disconnect;
The next commit will add support for the optional extension NBD_CMD_FLAG_PAYLOAD during NBD_CMD_BLOCK_STATUS, where the client can request that the server only return a subset of negotiated contexts, rather than all contexts. To make that task easier, this patch populates the list of contexts to return on a per-command basis (for now, identical to the full set of negotiated contexts). Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: split out NBDMetaContexts refactoring to its own patch, track NBDRequests.contexts as a pointer rather than inline --- include/block/nbd.h | 1 + nbd/server.c | 22 +++++++++++++++++----- 2 files changed, 18 insertions(+), 5 deletions(-)