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

[Martin Kletzander]

On Mon, Jun 11, 2018 at 12:12:16PM +0200, Pavel Hrdina wrote:
>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); \
>    }
>

I haven't followed all the discussions, but won't this be a problem
Sukrit is talking about that we need to fix?  For `char **` the above
will just not work.

Just making sure we all understand each other.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180611/ff0a6fb3/attachment-0001.sig>