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

[Sukrit Bhatnagar]

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

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

but I am getting the errors:
*** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-virbuftest':
free(): invalid pointer: 0x00007ffc7be60d48 ***
...
*** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-commandtest':
free(): invalid pointer: 0x00007fff727583fc ***
...

I am not quite sure how to debug this. Am I missing something basic?

>
> Note: don't take the types and function names as something that actually
> exists and be used like that, it's just an example to show how it would
> work :).
>
> Pavel