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

[Martin Kletzander]

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.
>

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.

Have a nice day,
Martin

>Thanks,
>Sukrit
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- 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/20180609/d00e62b3/attachment-0001.sig>