On Mon, Jun 11, 2018 at 11:59:06PM +0530, Sukrit Bhatnagar wrote:
> On Mon, 11 Jun 2018 at 16:35, Pavel Hrdina <phrdina@xxxxxxxxxx> wrote:
> >
> > On Mon, Jun 11, 2018 at 12:59:11PM +0200, Martin Kletzander wrote:
> > > On Mon, Jun 11, 2018 at 12:53:47PM +0200, Erik Skultety wrote:
> > > > On Mon, Jun 11, 2018 at 12:43:59PM +0200, Martin Kletzander wrote:
> > > > > 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.
> > > >
> > > > In the very last sentence of Pavel's last reply he said that char ** would have
> > > > to be special-cased regardless ;).
> > > >
> > >
> > > Maybe that's why I shouldn't mix in such conversations. Because I'm blind :)
> > >
> > > Anyway if that is special-cased then we still need to make a special function
> > > for that, so it feels contradictory to the rest of the message. With Sukrit's
> > > variant 2 it wouldn't have to be. And it would still be as easy to use for
> > > other types as Pavel wants it to be.
> > >
> > > But don't get blocked on me ;)
> >
> > We would not need special function for list of string, we would need
> > only special typedef:
> >
> > typedef char **virStringList;
> >
> > VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
> >
> > VIR_AUTOPTR(virStringList) list = NULL;
>
> So, are we all agreeing on this one i.e. modifying the original designed
> macros to take `type` instead of `typePtr` as Pavel suggested?
I think we might want to get a few more thoughts on this, so I put more people
on CC to speed things up a little, let's see who'll respond.
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
