[libvirt] [GSoC] Code design for scalar and external types

[Erik Skultety]

On Fri, Jun 22, 2018 at 04:43:35AM +0530, Sukrit Bhatnagar wrote:
> On Mon, 18 Jun 2018 at 17:20, Andrea Bolognani <abologna at redhat.com> wrote:
> >
> > On Mon, 2018-06-18 at 11:52 +0100, Daniel P. Berrangé wrote:
> > > On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> > > > # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > > >     static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > > >     { \
> > > >         (func)(_ptr); \
> > > >     }
> > >
> > > For anything where we define the impl of (func) these two macros
> > > should never be used IMHO. We should just change the signature of
> > > the real functions so that accept a double pointer straight away,
> > > and thus not require the wrapper function. Yes, it this will
> > > require updating existing code, but such a design change is
> > > beneficial even without the cleanup macros, as it prevents the
> > > possbility of double frees. I'd suggest we just do this kind of
> > > change straightaway
> >
> > Agreed that we want to fix our own free functions.
> >
> > > > # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> > >
> > > I don't see the point in passing "type" in here we could simplify
> > > this:
> > >
> > >   #define VIR_AUTOFREE __attribute__((cleanup(virFree))
> > >
> > >   VIR_AUTOFREE char *foo = NULL;
> >
> > Passing the type was suggested earlier to make usage consistent
> > across all cases, eg.
> >
> >   VIR_AUTOFREE(char *) str = NULL;
> >   VIR_AUTOPTR(virDomain) dom = NULL;
> >
> > and IIRC we ended up agreeing that it was a good idea overall.
> >
> > > > # define VIR_AUTOPTR(type) \
> > > >     __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) VIR_AUTOPTR_TYPE_NAME(type)
> > > >
> > > >       The usage would not require our internal vir*Ptr types and would
> > > >       be easy to use with external types as well:
> > > >
> > > > VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
> > > >
> > > > VIR_AUTOPTR(virBitmap) map = NULL;
> > > >
> > > > VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
> > >
> > > Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
> > > this is a reasonable usage, because we don't control the signature
> > > of the libssh2_channel_free function.
> >
> > This is why I suggested we might want to have two sets of
> > macros, one for types we defined ourselves and one for types
> > we bring in from external libraries.
> >
> > > > VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
> > >
> > > With my example above
> > >
> > >    #define VIR_AUTOFREE_LIBSSH_CHANNEL VIR_AUTOFREE_FUNC(LIBSSH2_CHANNEL)
> > >
> > >    VIR_AUTOFREE_LIBSSH_CHANNEL LIBSSH_CHANNEL *chan = NULL;
> >
> > This makes the declaration needlessly verbose, since you're
> > repeating the type name twice; Pavel's approach would avoid
> > that.
>
> So, have we reached a decision about the design?
>
> Fixing the existing vir*Free functions to take double pointers seems good to me,
> as many have mentioned earlier. It will solve multiple problems at once.
> I have almost 7 weeks left of GSoC, and I think it is totally doable.
>
> On the other hand, making cleanup macros use double pointers also seems
> great! But, aren't we borrowing too much design from GLib?
>
> Anyway, both approaches take care of the double free problem.
>
> TL;DR
> I prefer cleanup macros which define functions with double pointers
> over changing
> existing vir*Free functions. It just looks more natural.

It does, but it doesn't solve all the problems, mainly those Dan has in mind.
With attribute cleanup, it's applied on variables that go out of scope, but
that's not what you want if you want to keep the memory for while, imagine the
following:

A->B

where B:

<return type> B (<ptrptr> dst, ...)
{
    VIR_AUTOPTR(tmp);

    VIR_ALLOC(tmp)

    ...

    *dst = tmp;
    tmp = NULL;

    return;
}

when A is finished with dst, it will need to free it manually --> this could
lead to a double free if A doesn't clear it afterwards, either in A or some
within some other consumer of dst.

So in the long term, we'll probably want to change the signatures, but as I
said, I see the proposed macros as an improvement worth being a stepping stone
towards changing the signatures of free functions. The ultimate goal is still
to gradually rewrite libvirt into some other memory-safe language.

Erik