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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
