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 <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.
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 <ebl...@redhat.com>
>
--
Best regards,
Vladimir
