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

[Andrea Bolognani]

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.

-- 
Andrea Bolognani / Red Hat / Virtualization