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

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