Re: [Libguestfs] [PATCH v4 23/24] nbd/server: Prepare for per-request filtering of BLOCK_STATUS

2023-06-27 Thread Vladimir Sementsov-Ogievskiy 

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 
---




+++ 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 = >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, , req->data, _err);
  }
+if (request.type == NBD_CMD_BLOCK_STATUS &&
+request.contexts != >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 != >contexts) {
  g_free(..)
  g_free(..)
}


--
Best regards,
Vladimir

Re: [Libguestfs] [PATCH v4 23/24] nbd/server: Prepare for per-request filtering of BLOCK_STATUS

2023-06-08 Thread Eric Blake 
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 
> ---
> 

> +++ 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 = >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, , req->data, _err);
>  }
> +if (request.type == NBD_CMD_BLOCK_STATUS &&
> +request.contexts != >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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[PATCH v4 23/24] nbd/server: Prepare for per-request filtering of BLOCK_STATUS

2023-06-08 Thread Eric Blake 
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 
---

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(-)

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 = >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, , req->data, _err);
 }
+if (request.type == NBD_CMD_BLOCK_STATUS &&
+request.contexts != >contexts) {
+g_free(request.contexts->bitmaps);
+g_free(request.contexts);
+}
 if (ret < 0) {
 error_prepend(_err, "Failed to send reply: ");
 goto disconnect;
-- 
2.40.1