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

Thu May 24 19:23:06 UTC 2018

On Thu, May 24, 2018 at 11:16:40PM +0530, Sukrit Bhatnagar wrote:
> 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);

Yes, this solution would work but it looks way too complicated.
It would require to create new macro for every structure that we have
in libvirt which seems redundant if we can have few universal macros
that can be used for everything.

Each macro should be simple and should do one thing and should not hide
too much logic.

The design that I was proposing would be used like this for the
virCommandPtr:

src/util/viralloc.h:

#define VIR_AUTOFREE __attribute__((cleanup(virFree)))
#define VIR_AUTOPTR(type) ...
#define VIR_AUTOCLEAR(type) ...
#define VIR_DEFINE_AUTOPTR_FUNC(type, freeFunc) ...
#define VIR_DEFINE_AUTOCLEAR_FUNC(type, clearFunc) ...

src/util/vircommand.h:

VIR_DEFINE_AUTOPTR_FUNC(virCommandPtr, virCommandFree);

src/qemu/qemu_driver.c: (the virCommandPtr is used there)

    ...
    VIR_AUTOPTR(virCommandPtr) cmd = NULL;
    ...

Now the fist design would require to introduce a lot of new macros to
achieve the same result, let's summarize it for comparison:

src/util/vircommand.h:

#define VIRCOMMAND_AUTOFREE __attribute__((__cleanup__(virCommandFree))
#define virCommandAutoFreePtr VIRCOMMAND_AUTOFREE virCommandPtr

//optional
#define CREATE_CMD_PTR(var, val) \
    VIRCOMMAND_AUTOFREE virCommandPtr var = (val);

src/qemu/qemu_driver.c:

    ...
    virCommandAutoFreePtr cmd = NULL;
    ...

    or

    ...
    CREATE_CMD_PTR(cmd, virCommandNewArgs(args))
    ...

As you can see, the original solution doesn't have any generic macro
but it requires at least 2 new macros for every single structure
whereas the new design requires only 1 call of existing macro for every
structure.  It's more flexible and all the logic is only in once place.

> 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?

virDomainFree is a public API and we don't have to use them in our
internal code so this should not be an issue.  There are some internal
free functions that also return some value, for example virObjectUnref,
but that is not an issue as well.

Look at the documentation of GLib functions and also at the code to get
the idea of how it works.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180524/7eb2e18e/attachment-0001.sig>