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

[Eric Blake <eblake@redhat.com>]

On 10/18/19 11:07 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>    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;
> 
> ?
> 

No, that makes it worse.  It's that much more confusing to track who is 
allocating what and where it gets cleaned up.

I personally don't see the need to avoid jumping through hoops to avoid 
an in-out parameter (if we're going to rework code later, we'll notice 
that we documented how things are supposed to be used), but if in-out 
parameters bother you, then the approach you used, even with an extra 
memdup(), is the simplest way to maintain, even if it is not the most 
efficient.

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