Introduce NBDExtentArray class, to handle extents list creation in more
controlled way and with less OUT parameters in functions.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
nbd/server.c | 201 ++++++++++++++++++++++++++++-----------------------
1 file changed, 109 insertions(+), 92 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index a4b348eb32..cc722adc31 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1909,27 +1909,89 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
return ret;
}
+typedef struct NBDExtentArray {
+ NBDExtent *extents;
+ unsigned int nb_alloc;
+ unsigned int count;
+ uint64_t total_length;
+ bool converted; /* extents are converted to BE, no more changes allowed */
+} NBDExtentArray;
+
+static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc)
+{
+ NBDExtentArray *ea = g_new0(NBDExtentArray, 1);
+
+ ea->nb_alloc = nb_alloc;
+ ea->extents = g_new(NBDExtent, nb_alloc);
+
+ return ea;
+}
+
+static void nbd_extent_array_free(NBDExtentArray *ea)
+{
+ g_free(ea->extents);
+ g_free(ea);
+}
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
+
+/* Further modifications of the array after conversion are abandoned */
+static void nbd_extent_array_convert_to_be(NBDExtentArray *ea)
+{
+ int i;
+
+ if (ea->converted) {
+ return;
+ }
+ ea->converted = true;
+
+ for (i = 0; i < ea->count; i++) {
+ ea->extents[i].flags = cpu_to_be32(ea->extents[i].flags);
+ ea->extents[i].length = cpu_to_be32(ea->extents[i].length);
+ }
+}
+
/*
- * Populate @extents from block status. Update @bytes to be the actual
- * length encoded (which may be smaller than the original), and update
- * @nb_extents to the number of extents used.
- *
- * Returns zero on success and -errno on bdrv_block_status_above failure.
+ * Add extent to NBDExtentArray. If extent can't be added (no available space),
+ * return -1.
+ * For safety, when returning -1 for the first time, the array is converted
+ * to BE and further modifications are abandoned.
*/
-static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
- uint64_t *bytes, NBDExtent *extents,
- unsigned int *nb_extents)
+static int nbd_extent_array_add(NBDExtentArray *ea,
+ uint32_t length, uint32_t flags)
{
- uint64_t remaining_bytes = *bytes;
- NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
- bool first_extent = true;
+ assert(!ea->converted);
+
+ if (!length) {
+ return 0;
+ }
+
+ /* Extend previous extent if flags are the same */
+ if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
+ ea->extents[ea->count - 1].length += length;
+ ea->total_length += length;
+ return 0;
+ }
+
+ if (ea->count >= ea->nb_alloc) {
+ nbd_extent_array_convert_to_be(ea);
+ return -1;
+ }
+
+ ea->total_length += length;
+ ea->extents[ea->count] = (NBDExtent) {.length = length, .flags = flags};
+ ea->count++;
- assert(*nb_extents);
- while (remaining_bytes) {
+ return 0;
+}
+
+static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
+ uint64_t bytes, NBDExtentArray *ea)
+{
+ while (bytes) {
uint32_t flags;
int64_t num;
- int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
- &num, NULL, NULL);
+ int ret = bdrv_block_status_above(bs, NULL, offset, bytes, &num,
+ NULL, NULL);
if (ret < 0) {
return ret;
@@ -1938,60 +2000,37 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
(ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
- if (first_extent) {
- extent->flags = flags;
- extent->length = num;
- first_extent = false;
- } else if (flags == extent->flags) {
- /* extend current extent */
- extent->length += num;
- } else {
- if (extent + 1 == extents_end) {
- break;
- }
-
- /* start new extent */
- extent++;
- extent->flags = flags;
- extent->length = num;
+ if (nbd_extent_array_add(ea, num, flags) < 0) {
+ return 0;
}
- offset += num;
- remaining_bytes -= num;
- }
-
- extents_end = extent + 1;
- for (extent = extents; extent < extents_end; extent++) {
- extent->flags = cpu_to_be32(extent->flags);
- extent->length = cpu_to_be32(extent->length);
+ offset += num;
+ bytes -= num;
}
- *bytes -= remaining_bytes;
- *nb_extents = extents_end - extents;
-
return 0;
}
-/* nbd_co_send_extents
+/*
+ * nbd_co_send_extents
*
- * @length is only for tracing purposes (and may be smaller or larger
- * than the client's original request). @last controls whether
- * NBD_REPLY_FLAG_DONE is sent. @extents should already be in
- * big-endian format.
+ * @ea is converted to BE by the function
+ * @last controls whether NBD_REPLY_FLAG_DONE is sent.
*/
static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
- NBDExtent *extents, unsigned int nb_extents,
- uint64_t length, bool last,
- uint32_t context_id, Error **errp)
+ NBDExtentArray *ea,
+ bool last, uint32_t context_id, Error **errp)
{
NBDStructuredMeta chunk;
-
struct iovec iov[] = {
{.iov_base = &chunk, .iov_len = sizeof(chunk)},
- {.iov_base = extents, .iov_len = nb_extents * sizeof(extents[0])}
+ {.iov_base = ea->extents, .iov_len = ea->count * sizeof(ea->extents[0])}
};
- trace_nbd_co_send_extents(handle, nb_extents, context_id, length, last);
+ nbd_extent_array_convert_to_be(ea);
+
+ trace_nbd_co_send_extents(handle, ea->count, context_id, ea->total_length,
+ last);
set_be_chunk(&chunk.h, last ? NBD_REPLY_FLAG_DONE : 0,
NBD_REPLY_TYPE_BLOCK_STATUS,
handle, sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
@@ -2009,39 +2048,27 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
{
int ret;
unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
- NBDExtent *extents = g_new(NBDExtent, nb_extents);
- uint64_t final_length = length;
+ g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
- ret = blockstatus_to_extents(bs, offset, &final_length, extents,
- &nb_extents);
+ ret = blockstatus_to_extents(bs, offset, length, ea);
if (ret < 0) {
- g_free(extents);
return nbd_co_send_structured_error(
client, handle, -ret, "can't get block status", errp);
}
- ret = nbd_co_send_extents(client, handle, extents, nb_extents,
- final_length, last, context_id, errp);
-
- g_free(extents);
-
- return ret;
+ return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
}
/*
- * Populate @extents from a dirty bitmap. Unless @dont_fragment, the
- * final extent may exceed the original @length. Store in @length the
- * byte length encoded (which may be smaller or larger than the
- * original), and return the number of extents used.
+ * Populate @ea from a dirty bitmap. Unless @dont_fragment, the
+ * final extent may exceed the original @length.
*/
-static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
- uint64_t *length, NBDExtent *extents,
- unsigned int nb_extents,
- bool dont_fragment)
+static void bitmap_to_extents(BdrvDirtyBitmap *bitmap,
+ uint64_t offset, uint64_t length,
+ NBDExtentArray *ea, bool dont_fragment)
{
uint64_t begin = offset, end = offset;
- uint64_t overall_end = offset + *length;
- unsigned int i = 0;
+ uint64_t overall_end = offset + length;
BdrvDirtyBitmapIter *it;
bool dirty;
@@ -2050,8 +2077,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
it = bdrv_dirty_iter_new(bitmap);
dirty = bdrv_dirty_bitmap_get_locked(bitmap, offset);
- assert(begin < overall_end && nb_extents);
- while (begin < overall_end && i < nb_extents) {
+ while (begin < overall_end) {
bool next_dirty = !dirty;
if (dirty) {
@@ -2071,9 +2097,10 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
end = overall_end;
}
- extents[i].length = cpu_to_be32(end - begin);
- extents[i].flags = cpu_to_be32(dirty ? NBD_STATE_DIRTY : 0);
- i++;
+ if (nbd_extent_array_add(ea, end - begin,
+ dirty ? NBD_STATE_DIRTY : 0) < 0) {
+ break;
+ }
begin = end;
dirty = next_dirty;
}
@@ -2083,8 +2110,6 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
bdrv_dirty_bitmap_unlock(bitmap);
assert(offset < end);
- *length = end - offset;
- return i;
}
static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
@@ -2092,20 +2117,12 @@ static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
uint32_t length, bool dont_fragment, bool last,
uint32_t context_id, Error **errp)
{
- int ret;
unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
- NBDExtent *extents = g_new(NBDExtent, nb_extents);
- uint64_t final_length = length;
+ g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
- nb_extents = bitmap_to_extents(bitmap, offset, &final_length, extents,
- nb_extents, dont_fragment);
+ bitmap_to_extents(bitmap, offset, length, ea, dont_fragment);
- ret = nbd_co_send_extents(client, handle, extents, nb_extents,
- final_length, last, context_id, errp);
-
- g_free(extents);
-
- return ret;
+ return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
}
/* nbd_co_receive_request
--
2.21.0
On 12/19/19 4:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> Introduce NBDExtentArray class, to handle extents list creation in more
> controlled way and with less OUT parameters in functions.
s/less/fewer/
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> nbd/server.c | 201 ++++++++++++++++++++++++++++-----------------------
> 1 file changed, 109 insertions(+), 92 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index a4b348eb32..cc722adc31 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1909,27 +1909,89 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
> return ret;
> }
>
> +typedef struct NBDExtentArray {
> + NBDExtent *extents;
> + unsigned int nb_alloc;
> + unsigned int count;
> + uint64_t total_length;
> + bool converted; /* extents are converted to BE, no more changes allowed */
> +} NBDExtentArray;
> +
Looks good.
> +static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc)
> +{
> + NBDExtentArray *ea = g_new0(NBDExtentArray, 1);
> +
> + ea->nb_alloc = nb_alloc;
> + ea->extents = g_new(NBDExtent, nb_alloc);
I guess g_new() is okay rather tahn g_new0, as long as we are careful
not to read that uninitialized memory.
> +
> + return ea;
> +}
> +
> +static void nbd_extent_array_free(NBDExtentArray *ea)
> +{
> + g_free(ea->extents);
> + g_free(ea);
> +}
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
> +
> +/* Further modifications of the array after conversion are abandoned */
> +static void nbd_extent_array_convert_to_be(NBDExtentArray *ea)
> +{
> + int i;
> +
> + if (ea->converted) {
> + return;
> + }
Would this be better as assert(!ea->converted), to ensure we aren't
buggy in our usage? ...
> + ea->converted = true;
> +
> + for (i = 0; i < ea->count; i++) {
> + ea->extents[i].flags = cpu_to_be32(ea->extents[i].flags);
> + ea->extents[i].length = cpu_to_be32(ea->extents[i].length);
> + }
> +}
> +
> /*
> - * Populate @extents from block status. Update @bytes to be the actual
> - * length encoded (which may be smaller than the original), and update
> - * @nb_extents to the number of extents used.
> - *
> - * Returns zero on success and -errno on bdrv_block_status_above failure.
> + * Add extent to NBDExtentArray. If extent can't be added (no available space),
> + * return -1.
> + * For safety, when returning -1 for the first time, the array is converted
> + * to BE and further modifications are abandoned.
> */
> -static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
> - uint64_t *bytes, NBDExtent *extents,
> - unsigned int *nb_extents)
> +static int nbd_extent_array_add(NBDExtentArray *ea,
> + uint32_t length, uint32_t flags)
> {
> - uint64_t remaining_bytes = *bytes;
> - NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
> - bool first_extent = true;
> + assert(!ea->converted);
...especially since you assert here.
> +
> + if (!length) {
> + return 0;
> + }
> +
> + /* Extend previous extent if flags are the same */
> + if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
> + ea->extents[ea->count - 1].length += length;
> + ea->total_length += length;
> + return 0;
> + }
The NBD spec states that NBD_CMD_BLOCK_STATUS with flag
NBD_CMD_FLAG_REQ_ONE must not exceed the original length of the client's
request, but that when the flag is not present, the final extent may
indeed go beyond the client's request. I see two potential problems here:
1) I don't see any check that extending .length does not exceed the
client's request if NBD_CMD_FLAG_REQ_ONE was set (we can sort of tell if
that is the case based on whether nb_alloc is 1 or greater than 1, but
not directly here, and it seems like this is a better place to do a
common check than to make each caller repeat it).
2) I don't see any check that extending .length does not exceed 32 bits.
If the client requested status of 3.5G, but the caller divides that
into two extent additions of 3G each and with the same flags, we could
end up overflowing the 32-bit reply field (not necessarily fatal except
when the overflow is exactly at 4G, because as long as the server is
making progress, the client will eventually get all data; it is only
when the overflow wraps to exactly 0 that we quit making progress).
32-bit overflow is one case where the server HAS to return back-to-back
extents with the same flags (if it is going to return information on
that many bytes, rather than truncating its reply to just the first
extent < 4G).
> +
> + if (ea->count >= ea->nb_alloc) {
> + nbd_extent_array_convert_to_be(ea);
> + return -1;
> + }
> +
> + ea->total_length += length;
> + ea->extents[ea->count] = (NBDExtent) {.length = length, .flags = flags};
> + ea->count++;
>
> - assert(*nb_extents);
> - while (remaining_bytes) {
> + return 0;
> +}
> +
> +static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
> + uint64_t bytes, NBDExtentArray *ea)
> +{
> + while (bytes) {
> uint32_t flags;
> int64_t num;
> - int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
> - &num, NULL, NULL);
> + int ret = bdrv_block_status_above(bs, NULL, offset, bytes, &num,
> + NULL, NULL);
>
> if (ret < 0) {
> return ret;
> @@ -1938,60 +2000,37 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
> flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
> (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
>
> - if (first_extent) {
> - extent->flags = flags;
> - extent->length = num;
> - first_extent = false;
> - } else if (flags == extent->flags) {
> - /* extend current extent */
> - extent->length += num;
> - } else {
> - if (extent + 1 == extents_end) {
> - break;
> - }
> -
> - /* start new extent */
> - extent++;
> - extent->flags = flags;
> - extent->length = num;
> + if (nbd_extent_array_add(ea, num, flags) < 0) {
> + return 0;
> }
> - offset += num;
> - remaining_bytes -= num;
> - }
However, I _do_ like the refactoring on making the rest of the code
easier to read.
> -
> - extents_end = extent + 1;
>
> - for (extent = extents; extent < extents_end; extent++) {
> - extent->flags = cpu_to_be32(extent->flags);
> - extent->length = cpu_to_be32(extent->length);
> + offset += num;
> + bytes -= num;
> }
>
> - *bytes -= remaining_bytes;
> - *nb_extents = extents_end - extents;
> -
> return 0;
> }
>
I think this needs v4 to fix the boundary cases, but I like where it is
headed.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
20.01.2020 23:20, Eric Blake wrote:
> On 12/19/19 4:03 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Introduce NBDExtentArray class, to handle extents list creation in more
>> controlled way and with less OUT parameters in functions.
>
> s/less/fewer/
>
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> nbd/server.c | 201 ++++++++++++++++++++++++++++-----------------------
>> 1 file changed, 109 insertions(+), 92 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index a4b348eb32..cc722adc31 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -1909,27 +1909,89 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
>> return ret;
>> }
>> +typedef struct NBDExtentArray {
>> + NBDExtent *extents;
>> + unsigned int nb_alloc;
>> + unsigned int count;
>> + uint64_t total_length;
>> + bool converted; /* extents are converted to BE, no more changes allowed */
>> +} NBDExtentArray;
>> +
>
> Looks good.
>
>> +static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc)
>> +{
>> + NBDExtentArray *ea = g_new0(NBDExtentArray, 1);
>> +
>> + ea->nb_alloc = nb_alloc;
>> + ea->extents = g_new(NBDExtent, nb_alloc);
>
> I guess g_new() is okay rather tahn g_new0, as long as we are careful not to read that uninitialized memory.
>
>> +
>> + return ea;
>> +}
>> +
>> +static void nbd_extent_array_free(NBDExtentArray *ea)
>> +{
>> + g_free(ea->extents);
>> + g_free(ea);
>> +}
>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
>> +
>> +/* Further modifications of the array after conversion are abandoned */
>> +static void nbd_extent_array_convert_to_be(NBDExtentArray *ea)
>> +{
>> + int i;
>> +
>> + if (ea->converted) {
>> + return;
>> + }
>
> Would this be better as assert(!ea->converted), to ensure we aren't buggy in our usage? ...
No, as array may be already automatically converted by nbd_extent_array_add, or may be not.
But your question stress that my design is weird. Now I think it's better to add separate
boolean ea field for nbd_extent_array_add() safety, instead of reusing .converted.
>
>> + ea->converted = true;
>> +
>> + for (i = 0; i < ea->count; i++) {
>> + ea->extents[i].flags = cpu_to_be32(ea->extents[i].flags);
>> + ea->extents[i].length = cpu_to_be32(ea->extents[i].length);
>> + }
>> +}
>> +
>> /*
>> - * Populate @extents from block status. Update @bytes to be the actual
>> - * length encoded (which may be smaller than the original), and update
>> - * @nb_extents to the number of extents used.
>> - *
>> - * Returns zero on success and -errno on bdrv_block_status_above failure.
>> + * Add extent to NBDExtentArray. If extent can't be added (no available space),
>> + * return -1.
>> + * For safety, when returning -1 for the first time, the array is converted
>> + * to BE and further modifications are abandoned.
>> */
>> -static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
>> - uint64_t *bytes, NBDExtent *extents,
>> - unsigned int *nb_extents)
>> +static int nbd_extent_array_add(NBDExtentArray *ea,
>> + uint32_t length, uint32_t flags)
>> {
>> - uint64_t remaining_bytes = *bytes;
>> - NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
>> - bool first_extent = true;
>> + assert(!ea->converted);
>
> ...especially since you assert here.
>
>> +
>> + if (!length) {
>> + return 0;
>> + }
>> +
>> + /* Extend previous extent if flags are the same */
>> + if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
>> + ea->extents[ea->count - 1].length += length;
>> + ea->total_length += length;
>> + return 0;
>> + }
>
> The NBD spec states that NBD_CMD_BLOCK_STATUS with flag NBD_CMD_FLAG_REQ_ONE must not exceed the original length of the client's request, but that when the flag is not present, the final extent may indeed go beyond the client's request. I see two potential problems here:
>
> 1) I don't see any check that extending .length does not exceed the client's request if NBD_CMD_FLAG_REQ_ONE was set (we can sort of tell if that is the case based on whether nb_alloc is 1 or greater than 1, but not directly here, and it seems like this is a better place to do a common check than to make each caller repeat it).
we have two callers. blockstatus_to_extents can't exceed the requested range, and bitmaps_to_extents has own check. If we want to move the check into nbd_extent_array_add, we need to enhance its interface to allow it to add only "part" of extent.. And how to handle it?
Mark the array "closed" after first partly applied extent, but return success? Then we'll have to change assertion at start of _add s/assert(ea->can_add)/if (!ea->can_add) {return -1}/.. Or return count of really applied bytes to the caller?
I doubt that this is a good idea, it seems simpler to keep nbd extent array not knowing about length limitation, keeping in mind that the following patch will drop any exceeding of the requested range.
>
> 2) I don't see any check that extending .length does not exceed 32 bits. If the client requested status of 3.5G, but the caller divides that into two extent additions of 3G each and with the same flags, we could end up overflowing the 32-bit reply field (not necessarily fatal except when the overflow is exactly at 4G, because as long as the server is making progress, the client will eventually get all data; it is only when the overflow wraps to exactly 0 that we quit making progress). 32-bit overflow is one case where the server HAS to return back-to-back extents with the same flags (if it is going to return information on that many bytes, rather than truncating its reply to just the first extent < 4G).
good catch. I'll write it like
/* Extend previous extent if flags are the same */
if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
uint64_t sum = (uint64_t)length + ea->extents[ea->count - 1].length;
if (sum <= UINT32_MAX) {
ea->extents[ea->count - 1].length = sum;
ea->total_length += length;
return 0;
}
}
>
>> +
>> + if (ea->count >= ea->nb_alloc) {
>> + nbd_extent_array_convert_to_be(ea);
>> + return -1;
>> + }
>> +
>> + ea->total_length += length;
>> + ea->extents[ea->count] = (NBDExtent) {.length = length, .flags = flags};
>> + ea->count++;
>> - assert(*nb_extents);
>> - while (remaining_bytes) {
>> + return 0;
>> +}
>> +
>> +static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
>> + uint64_t bytes, NBDExtentArray *ea)
>> +{
>> + while (bytes) {
>> uint32_t flags;
>> int64_t num;
>> - int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
>> - &num, NULL, NULL);
>> + int ret = bdrv_block_status_above(bs, NULL, offset, bytes, &num,
>> + NULL, NULL);
>> if (ret < 0) {
>> return ret;
>> @@ -1938,60 +2000,37 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
>> flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
>> (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
>> - if (first_extent) {
>> - extent->flags = flags;
>> - extent->length = num;
>> - first_extent = false;
>> - } else if (flags == extent->flags) {
>> - /* extend current extent */
>> - extent->length += num;
>> - } else {
>> - if (extent + 1 == extents_end) {
>> - break;
>> - }
>> -
>> - /* start new extent */
>> - extent++;
>> - extent->flags = flags;
>> - extent->length = num;
>> + if (nbd_extent_array_add(ea, num, flags) < 0) {
>> + return 0;
>> }
>> - offset += num;
>> - remaining_bytes -= num;
>> - }
>
> However, I _do_ like the refactoring on making the rest of the code easier to read.
>
>> -
>> - extents_end = extent + 1;
>> - for (extent = extents; extent < extents_end; extent++) {
>> - extent->flags = cpu_to_be32(extent->flags);
>> - extent->length = cpu_to_be32(extent->length);
>> + offset += num;
>> + bytes -= num;
>> }
>> - *bytes -= remaining_bytes;
>> - *nb_extents = extents_end - extents;
>> -
>> return 0;
>> }
>
> I think this needs v4 to fix the boundary cases, but I like where it is headed.
>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.