On 9/30/19 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
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 <vsement...@virtuozzo.com>
---
nbd/server.c | 184 +++++++++++++++++++++++++--------------------------
1 file changed, 90 insertions(+), 94 deletions(-)
+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);
Nice to see this getting more popular :)
+
+static int nbd_extent_array_add(NBDExtentArray *ea,
+ uint32_t length, uint32_t flags)
{
- assert(*nb_extents);
- while (remaining_bytes) {
+ if (ea->count >= ea->nb_alloc) {
+ return -1;
+ }
Returning -1 is not a failure in the protocol, just failure to add any
more information to the reply. A function comment might help, but this
looks like a good helper function.
+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 (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;
Also looks good (return 0 if we populated until we either ran out of
reply space or out of bytes to report on).
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;
-
+ size_t len = ea->count * sizeof(ea->extents[0]);
+ g_autofree NBDExtent *extents = g_memdup(ea->extents, len);
Why do we need memdup here? What's wrong with modifying ea->extents in
place?...
+ NBDExtent *extent, *extents_end = extents + ea->count;
struct iovec iov[] = {
{.iov_base = &chunk, .iov_len = sizeof(chunk)},
- {.iov_base = extents, .iov_len = nb_extents * sizeof(extents[0])}
+ {.iov_base = extents, .iov_len = len}
};
- trace_nbd_co_send_extents(handle, nb_extents, context_id, length, last);
+ for (extent = extents; extent < extents_end; extent++) {
+ extent->flags = cpu_to_be32(extent->flags);
+ extent->length = cpu_to_be32(extent->length);
+ }
+
+ 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);
@@ -1994,39 +2012,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);
...especially since ea goes out of scope right after the helper function
finishes?
Overall looks like a nice refactoring.
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org