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

[Sukrit Bhatnagar]

On 28 May 2018 at 13:54, Pavel Hrdina <phrdina at redhat.com> wrote:
> 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)
>
> NACK to this, please look few lines above how the macros should be named
> and which macros we would like to have implemented.
>
> There is no need to have the _VIR* helper macros and you need to
> implement the VIR_DEFINE_* macros.
>
>> The problem is that our vir*Free functions take on vir*Ptr as the
>> parameter and not
>> vir*Ptr * (pointer to it).
>
> The problem is only with your current implementation, the original
> design should not have this issue.
>
> The part of VIR_DEFINE_* macros is definition of new wrapper function
> for the cleanup function which means that our existing free functions
> are not used directly.
>
> GLib version of the define macro:
>
> #define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \
>     typedef TypeName *_GLIB_AUTOPTR_TYPENAME(TypeName); \
>     G_GNUC_BEGIN_IGNORE_DEPRECATIONS \
>     static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) \
>     { \
>         if (*_ptr) \
>             (func) (*_ptr); \
>     } \
>     G_GNUC_END_IGNORE_DEPRECATIONS
>
> Obviously we don't need the "typedef" line and the DEPRECATIONS macros.


So, using the discussed design, I have added the following lines:

# define VIR_AUTOPTR_FUNC_NAME(type) type##Free
# define VIR_AUTOCLEAR_FUNC_NAME(type) type##Clear

# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
    static inline void VIR_AUTOPTR_FUNC_NAME(type) (type *_ptr) \
    { \
        if (*_ptr) \
            (func) (*_ptr); \
    } \

# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
    static inline void VIR_AUTOCLEAR_FUNC_NAME(type) (type *_ptr) \
    { \
        if (*_ptr) \
            (func) (*_ptr); \
    } \

# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
# define VIR_AUTOPTR(type) \
    __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
# define VIR_AUTOCLEAR(type) \
    __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type


We need to add something like this for each type to autofree:
VIR_DEFINE_AUTOFREE_FUNC(virArpTablePtr, virArpTableFree)

Then we would use it like:
VIR_AUTOFREEE(void *) nlData = NULL;
VIR_AUTOPTR(virArpTablePtr) table = NULL;

Does this seem fine?

Also, I am still getting the errors by doing the above:
...
*** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-virbuftest':
free(): invalid pointer: 0x00007ffdd073a208 ***
...
*** Error in `/home/skrtbhtngr/libvirt/tools/.libs/lt-virsh':
free(): invalid pointer: 0x00007fc42349dcf9 ***
...

I am not changing or adding a new function (beside the one defined in macro),
just using original virArpTableFree.


Also, since I am defining these in viralloc.h, we would need to
include ALL header
files where *Ptr typedefs are made.

We can do this in three ways:
- Include all the needed header files in viralloc.h and invoke all
VIR_DEFINE_AUTOFREE_FUNC macros in viralloc.h itself.
- We make a separate file for VIR_DEFINE_AUTOFREE_FUNC invocations
where all the required *Ptr headers can be included. Then directly include that
new header file in all the relevant .c files.
- We invoke VIR_DEFINE_AUTOFREE_FUNC in respective header files,
for example, invoke:

VIR_DEFINE_AUTOFREE_FUNC(virArpTablePtr, virArpTableFree)

in virarptable.h, and also include viralloc.h in it. Then, we do not need extra
includes in .c files.