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