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

On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote: > On 25 May 2018 at 16:20, Pavel Hrdina <phrdina(a)redhat.com&gt; 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.