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

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); \ }