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

[Eric Blake]

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