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

[Vladimir Sementsov-Ogievskiy]

09.10.2019 20:02, Eric Blake wrote:
> 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 <address@hidden>
>> ---
>>   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.

Something like
/*
  * Add extent to NBDExtentArray. If extent can't be added (no available space),
  * return -1.
  */
above the 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?...

To not make ea to be IN-OUT parameter.. I don't like functions with side 
effects.
It will break the code if at some point we call nbd_co_send_extents twice on 
same
ea object.

What is the true way? To not memdup, nbd_co_send_extents should consume the 
whole
ea object..

Seems, g_autoptr attribute can't be used for function parameter, gcc complains:
nbd/server.c:1983:32: error: ‘cleanup’ attribute ignored [-Werror=attributes]
  1983 |                                g_autoptr(NBDExtentArray) ea,
       |                                ^~~~~~~~~

so, is it better
to call nbd_co_send_external(... g_steal_pointer(&ea) ...)

and than in nbd_co_send_external do

g_autoptr(NBDExtentArray) local_ea = ea;
NBDExtent *extents = local_ea->extents;

?


> 
>> +    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 <address@hidden>
> 


-- 
Best regards,
Vladimir