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

[Sukrit Bhatnagar]

On 23 May 2018 at 21:35, Pavel Hrdina <phrdina at redhat.com> wrote:
> On Sun, Mar 25, 2018 at 01:55:07AM +0530, Sukrit Bhatnagar wrote:
>> Hi,
>>
>> I am interested in implementing the GCC cleanup attribute for automatic
>> resource freeing as part of GSoC'18. I have shared a proposal for the same.
>>
>> This mail is to discuss the code design for implementing it.
>>
>>
>> Here are some of my ideas:
>>
>> This attribute requires a cleanup function that is called automatically
>> when the corresponding variable goes out of scope. There are some functions
>> whose logic can be reused:
>>
>> - Functions such as virCommandFree, virConfFreeList and virCgroupFree can
>> be directly used as cleanup functions. They have parameter and return type
>> valid for a cleanup function.
>>
>> - Functions such as virFileClose and virFileFclose need some additional
>> consideration as they return a value. I think we can set some global
>> variable in a separate source file (just like errno variable from errno.h).
>> Then the value to be returned can be accessed globally.
>>
>> - Functions such as virDomainEventGraphicsDispose need an entirely new
>> design. They are used as callbacks in object classes and passed as an
>> argument in virClassNew. This would require making changes to
>> virObjectUnref's code too. *This is the part I am not sure how to implement
>> cleanup logic for.*
>>
>>
>> Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro
>> like autoclean (ideas for macro name welcome!) can be used instead. As
>> Martin pointed out in my proposal, for some types, this can be done right
>> after typedef declarations, so that the type itself contains this attribute.
>>
>> Basically, at most places where VIR_FREE is used to release memory
>> explicitly, the corresponding variable can use the attribute. The existing
>> virFree function also can be reused as it takes void pointer as an argument
>> and returns nothing.
>> One of the exceptions to this will be those variables which are struct
>> members. The cleanup of member has to be done when the enclosing struct
>> variable is cleaned.
>>
>> I can create new files vircleanup.{c,h} for defining cleanup functions for
>> types which do not have an existing cleanup/free function. This can be done
>> separately for each driver supported.
>> For example, cleanups pertaining to lxc driver will be in
>> src/lxc/lxc_cleanup.c.
>
> Hi,
>
> I would like to apologize that it took me that long to reply.
>
> We've already discussed this a little bit off-list so I would like to
> summarize my idea about the design of this effort.
>
> I liked the way how GLib is solving the issue so we can simply use the
> same approach since it looks reasonable.
>
> There would be three different macros that would be used to annotate
> variable with attribute cleanup:
>
> VIR_AUTOFREE char *str = NULL;
>
>     - this would call virFree on that variable

This seems fine for basic native datatypes.

But for the complex types, why can't we do something like the following, as was
discussed previously?

* Define simple macros for the attribute per struct type:

#define VIRCOMMAND_AUTOFREE __attribute__((__cleanup__(freefunction))
#define VIRCOMMAND_AUTOCLEAN __attribute__((__cleanup__(cleanfunction))

* Create new datatypes which include this attribute:

#define virCommandAutoFreePtr VIRCOMMAND_AUTOFREE virCommandPtr
#define virCommandAutoCleanPtr VIRCOMMAND_AUTOCLEAN virCommandPtr

* Then simply declare variables as:

virCommandAutoFreePtr cmd = NULL;

We just have to make sure that all Ptr variables are initialized,
atleast to NULL.


Also, we can create and use macros to initialize the variables:

#define CREATE_CMD_PTR(var, val) \
    VIRCOMMAND_AUTOFREE virCommandPtr var = (val);
CREATE_CMD_PTR(cmd, virCommandNewArgs(args))

This is equivalent to:
VIRCOMMAND_AUTOFREE virCommandPtr cmd = virCommandNewArgs(args);


Many existing Free functions such as virDomainFree, return a value
which is not allowed for a function that is to be used with cleanup attribute.
Do we ignore all such return values?


> VIR_AUTOPTR(virDomain) domain = NULL;
>
>     - this would call registered free function on that variable
>     - to register the free function you would use:
>
>         VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
>
> VIR_AUTOCLEAR(virDomain) domain = { 0 };
>
>     - this would call registered clear function to free the content of
>       that structure
>     - to register that clear function you would use:
>
>         VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
>
> In GLib there are three "define" macros and all of them creates a
> wrapper function to make sure that the Free/Clear function is called
> only if necessary.  This is a safe-guard because it's a public API.
> We don't have to have this safe-guard in place since almost all of our
> Free functions already check whether the pointer is null or not.
>
> For reference how it works in GLib you can look here [1].
>
> Pavel
>
> [1] <https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html>