[libvirt] [GSoC] Code design for scalar and external types

[Erik Skultety]

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