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

[Eric Blake <eblake@redhat.com>]

On 2/5/20 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> Introduce NBDExtentArray class, to handle extents list creation in more
> controlled way and with fewer OUT parameters in functions.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   nbd/server.c | 210 +++++++++++++++++++++++++++++----------------------
>   1 file changed, 118 insertions(+), 92 deletions(-)
> 

> +
> +/* Further modifications of the array after conversion are abandoned */
> +static void nbd_extent_array_convert_to_be(NBDExtentArray *ea)
> +{
> +    int i;
> +
> +    assert(!ea->converted_to_be);

Comment is stale - further modifications after conversion are a bug that 
aborts the program, not abandoned.


>   /*
> - * 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, .can_add is set to false,
> + * further call to nbd_extent_array_add() will crash.

s/further call/so further calls/

> + * (to avoid the situation, when after failing to add an extent (returned -1),
> + * user miss this failure and add another extent, which is successfully added
> + * (array is full, but new extent may be squashed into the last one), then we
> + * have invalid array with skipped extent)

Long comment with nested ().  I'm not sure it adds much value, I think 
it can safely be dropped.  But if it is kept, I suggest:

(this ensures that after a failure, no further extents can accidentally 
change the bounds of the last extent in the array)

>    */
> -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->can_add);
> +
> +    if (!length) {
> +        return 0;
> +    }
> +
> +    /* 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) {
> +        ea->can_add = false;
> +        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;
> +}

Looks like you properly addressed my concerns from v3.

Comment changes are trivial, so you can add:

Reviewed-by: Eric Blake <eblake@redhat.com>

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