5:52 a.m.
On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
Hi,
It took me longer to sit down and write this mail but here it goes.
There was a lot of ideas about the macro design and it's easy to
get lost in all the designs.
So we agreed on this form:
# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
{ \
(func)(*_ptr); \
}
# 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
# 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;
and at the same time fix the char ** problems
#define VIR_AUTOFREE_FUNC(func) __attribute__((cleanup(func))
VIR_AUTOFREE_FUNC(virStringListFree) char *foo = NULL;
or if we want to simplify it
#define VIR_AUTOFREE_STRINGLIST VIR_AUTOFREE_FUNC(virStringListFree)
VIR_AUTOFREE_STRINGLIST char **strs = NULL;
This also works for external libraries
# define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
# define VIR_AUTOCLEAR(type) \
__attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
but it turned out the it's not sufficient for several reasons:
- there is no simple way ho to free list of strings (char **)
- this will not work for external types with their own free function
In order to solve these two issues there were some ideas.
1. How to handle list of strings
We have virStringListFree() function in order to free list of strings,
the question is how to use it with these macros:
- Create a new typedef for list of strings and use VIR_AUTOPTR:
typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
VIR_AUTOPTR(virStringList) list = NULL;
I don't think we should be creating artifical new typedefs just to
deal with the flawed design of our own autofree macros.
- Overload VIR_AUTOFREE macro by making it variadic
# define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
# define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
# define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
# define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__, _VIR_AUTOFREE_1,
_VIR_AUTOFREE_0)(__VA_ARGS__)
This is uneccessarily black magic - just have VIR_AUTOFREE and
VIR_AUTOFREE_FUNC defined separately.
void virStringListPtrFree(char ***stringsp)
{
virStringListFree(*stringsp);
}
As above, we should just fixed virStringListFree to have the better
signature straight away.
VIR_AUTOFREE(char **, virStringListPtrFree) list = NULL;
2. External types with free function
Libvirt uses a lot of external libraries where we use the types and
relevant free functions. The issue with original design is that it was
counting on the fact that we would have vir*Ptr typedefs, but that's not
the case for external types.
- We can modify the design to be closer to GLib design which would
work even with external types and would be safe in case external
free functions will not behave correctly. These are to
modification to the original design:
# define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
typedef type *VIR_AUTOPTR_TYPE_NAME(type);
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
{ \
if (*_ptr) { \
(func)(*_ptr); \
*_ptr = NULL; \
} \
}
# 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.
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;
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|