[libvirt] [GSoC] Design ideas for implementing cleanup attribute

[Erik Skultety]

On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
> On 25 May 2018 at 16:20, Pavel Hrdina <phrdina at redhat.com> wrote:
> > On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
> >> On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
> >> > On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
> >> > > However, I realize it might not be possible to register free
> >> > > functions for a native type without having to introduce something
> >> > > like
> >> > >
> >> > >   typedef char * virString;
> >> > >
> >> > > thus causing massive churn. How does GLib deal with that?
> >> >
> >> > If you would look into GLib documentation you would see that this
> >> > design basically copies the one in GLib:
> >>
> >> Sorry, I should have looked up the documentation and implementation
> >> before asking silly questions. Guess the morning coffee hadn't quite
> >> kicked in yet :/
> >>
> >> >     GLib                libvirt
> >> >
> >> >     g_autofree          VIR_AUTOFREE
> >> >     g_autoptr           VIR_AUTOPTR
> >> >     g_auto              VIR_AUTOCLEAR
> >>
> >> For what it's worth, I think VIR_AUTOCLEAR is a much better name
> >> than g_auto :)
> >>
> >> > In GLib you are using them like this:
> >> >
> >> > g_autofree char *string = NULL;
> >> > g_autoptr(virDomain) dom = NULL;
> >> > g_auto(virDomain) dom = { 0 };
> >> >
> >> > So yes it would require to introduce a lot of typedefs for basic types
> >> > and that is not worth it.
> >>
> >> I'm not sure we would need so many typedefs, but there would
> >> certainly be a lot of churn involved.
> >>
> >> Personally, I'm not so sure it wouldn't be worth the effort,
> >> but it's definitely something that we can experiment with it at
> >> a later time instead of holding up what's already a pretty
> >> significant improvement.
> >>
> >> > In libvirt we would have:
> >> >
> >> > VIR_AUTOFREE char *string = NULL;
> >> > VIR_AUTOPTR(virDomainPtr) dom = NULL;
> >> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
> >> >
> >> > If you notice the difference, in libvirt we can use virDomainPtr
> >> > directly because we have these typedefs, in GLib macro
> >> > G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
> >>
> >> While I'm not a fan of our *Ptr typedefs in general, I guess this
> >> time I'm glad we have them because VIR_AUTOPTR() doesn't hide the
> >> fact that what you're declaring is a pointer; that is, the macro
> >> argument is also exactly the type of the variable.
> >
> > So let's make a summary of how it could look like:
> >
> > VIR_AUTOFREE(char *) string = NULL;
> > VIR_AUTOPTR(virDomainPtr) vm = NULL;
> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
> >
> > VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree);
> > VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
>
> Do we define new functions for freeing/clearing, because that is what
> VIR_DEFINE_AUTOFREE_FUNC seems to do.
>
>
> This is what new macros will look like:
>
> # define _VIR_TYPE_PTR(type) type##Ptr
>
> # define _VIR_ATTR_AUTOFREE_PTR(type)  __attribute__((cleanup(type##Free)))
> # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close)))
> # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
>
> # define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type)
>
>
> The problem is that our vir*Free functions take on vir*Ptr as the
> parameter and not
> vir*Ptr * (pointer to it).
>
> For example, instead of:
> void virArpTableFree(virArpTablePtr table);
>
> we would need:
> void virArpTableFree(virArpTablePtr *table);


Hmm, so gcc claims the cleanup attribute to take a function which is required
to take a pointer to the type which it already is, it's just typedef'd, I
haven't tried this myself but do you get a compiler error if you try it with
the existing function, i.e. the one which doesn't mention the explicit '*' to
signal a pointer type? If so and we can't use the our typedef'd Ptr type
directly, then we'll need to switch all the free callback signatures to a plain
virSomeType *arg rather than trying to pass a double pointer which is just
unnecessary and ugly.

>
> if we declare something like:
> VIR_AUTOFREE_PTR(virArpTable) table = NULL;
>
>
> Also, I tried to add a new function:
> void virArpTablePtrFree(virArpTablePtr *table)
> {
>     size_t i;
>
>     if (!*table)
>         return;
>
>     for (i = 0; i < (*table)->n; i++) {
>         VIR_FREE((*table)->t[i].ipaddr);
>         VIR_FREE((*table)->t[i].mac);
>     }
>     VIR_FREE((*table)->t);
>     VIR_FREE((*table));
>     VIR_FREE(table);

My honest guess would be ^this you're trying to free the double pointer itself
which you didn't and you're also not supposed to allocate.

 Erik