On Sat, Jun 09, 2018 at 11:12:29PM +0200, Martin Kletzander wrote:
> 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.
> >
> > 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.
> >
> > Here we are specifying the Free function to use at the time of usage
> > of the VIR_AUTOFREE macro, which may make the code look bad:
> > VIR_AUTOFREE(char **, virStringListSomethingFree) lines = NULL;
> >
> >
> > Suggestions and opinions are welcome.
I don't like this solution for several reasons, first of all
VIR_AUTOFREE should be as simple as calling virFree(). If we decide for
this or similar solution, we should create a new macro, not overload
this one.
In order to use this we would have to create another free function for
each specific type anyway because the function passed to attribute
cleanup takes a pointer to the actual type so as Sukrit mentioned
virStringListFree would not be good enough and there would have to be
some wrapper for it.
> Just my two cents, but I like the second variant more. For few reasons:
>
> - If we typedef char ** to something, then all users of that function will need
> to cast it back to char ** since they will be accessing the underlying strings
> (char *), even if there is a macro or a function for that, it seems the code
> will be less readable.
>
> - We are using a trick similar to the second variant in tests/virmock.h,
> although for a different purpose. See VIR_MOCK_COUNT_ARGS
>
> - With the first approach we're going to have to create unnecessary types and
> possibly lot of them.
Yes, for each type we would have to create a new typePtr and that is not
nice so we might need to revise the design of VIR_AUTOPTR to take 'type'
instead of 'typePtr' and as GLib is doing and create the special typedef
only inside the macro and only for this purpose.
Another issue that we need to take into account is that the external
free functions might not be 'NULL' safe which we need to somehow ensure
if they will be used with attribute cleanup.
The original design is probably wrong and was heavily counting on the
existing libvirt implementation. We might need to update the design
to make it the similar as GLib:
#define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
#define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
#define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) VIR_AUTOPTR_TYPE_NAME(type)
#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); \
}
The usage would like this:
VIR_DEFINE_AUTOPTR_FUNC(virAuthConfig, virAuthConfigFree)
VIR_AUTOPTR(virAuthConfig) authConfig = NULL;
That way we can use any existing free function regardless whether it
checks if the variable is NULL or not and without the need to manually
define new type.
The only exception would be the virStringList where we would have to use
the different type.
Pavel
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
