Re: [libvirt] [GSoC] Code design for scalar and external types
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