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

[Vladimir Sementsov-Ogievskiy]

26.02.2020 18:06, Eric Blake wrote:
> 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 <address@hidden>
> > ---
> >   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.

I always thought that "abandoned" is something that must not be done, so the 
word works here. But I don't know English well).
May be:

"No further modifications of the array allowed after converting to BE."? Is it 
better?

Or just drop the comment.


> >   /*
> > - * 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)

works for me

> >    */
> > -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 <address@hidden>

Thanks!

--
Best regards,
Vladimir