Re: [libvirt] [GSoC] Code design for scalar and external types
On Sat, Jun 09, 2018 at 10:06:55PM +0530, Sukrit Bhatnagar wrote:
Hi,
I am starting this discussion thread as a continuation of my GSoC
weekly meeting with Erik and Pavel on 8th June.
I was going through src/util/virstring.c for adding cleanup macros and
saw that virStringListFree takes on char ** as an argument, and
equivalently, we declare a list of strings as char **.
For the cleanup function defined by VIR_DEFINE_AUTOPTR_FUNC, it is
required that the associated type has a name like virSomethingPtr.
It was also discussed that there are similar issues with DBus types,
but VIR_AUTOFREE can work there as we use VIR_ALLOC. I honestly don't
know much about that.
We discussed that we have two solutions:
- Create a virSomethingPtr by typedef-ing char**
As Pavel told, GLib has typedef gchar** GStrv; which is used together
with g_auto and it has g_strfreev(gchar **str_array) which is the same
as we have virStringListFree()
I have tried adding the following in src/util/virstrnig.h, and it
seems to work fine:
typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
We can use it as:
VIR_AUTOPTR(virStringList) lines = NULL;
There may be other scalar and external types where this problem
occurs, and it is not good to create a typedef for each of them, but
maybe we can make an exception for char ** and create a type for it.
- Overload VIR_AUTOFREE macro by making it variadic
As Erik told, we could make VIR_AUTOFREE a variadic macro whose
varying parameter can be the Free function name. If left blank, we use
virFree.
I went ahead with trying it and after reading some posts on
StackOverflow, I came up with this:
#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__)
The required functionality is working as expected; passing only one
argument will use virFree, and passing two arguments will use the
function specified as 2nd argument. Passing more than 2 arguments will
result in an error.
So, I had something slightly different from ^this in mind, but it turns out, I
was wrong about that and your approach to what I proposed is the correct
one, a bit cryptic, but that would be the case anyway, were my idea even
possible to implement, so nevermind.
Note: if the overall consensus will be in favour of the second approach, we'll
need to work on the helper macro naming though.
The macros with _ prefix are meant to be for internal use only.
Also, @func needs to be a wrapper around virStringListFree as it will
take char ***, not just char **. We probably need to define a new
function.
Not necessarily, you could create a static inline wrapper the same way as do for
the VIR_AUTOPTR macro. But I understand Pavel's concerns, that theoretically we
can ditch everything we've come up so far and have a universal
VIR_AUTOFREE/VIR_AUTOPTR for everything (with the __VA_ARGS__ bits included),
but you already know I feel the same way about this as Martin apparently does in
that we could end up with many more typedefs than we can currently think of
using the first approach, whereas here, yeah the macro is kinda hacky, but that
doesn't matter for the user of the macro (+ we've got more hacky ones) and
whether someone passes a custom function instead of a dedicated "XFree" wrapper
or not is the responsibility of the reviewer to point out as it's with many
other things, so we shouldn't even talk about a potential misuse of the macro
itself.
Erik