Re: [PATCH 08/10] nbd/server: introduce NBDExtentArray

[Eric Blake <eblake@redhat.com>]

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 <vsementsov@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 <eblake@redhat.com>

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