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

2018-06-01 Thread Daniel P . Berrangé 
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
> On 25 May 2018 at 16:20, Pavel Hrdina  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 :/
> >>
> >> > GLiblibvirt
> >> >
> >> > 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);

This is actually a *good* thing. Passing in 'Ptr *' allows the
virXXXFree function to set the pointer to NULL after free'ing
it which prevents a double-free.

If we need to change these free functions, that's a worthwhile
improvement in general IMHO.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-29 Thread Pavel Hrdina 
On Mon, May 28, 2018 at 09:40:41PM +0530, Sukrit Bhatnagar wrote:
> On 28 May 2018 at 13:54, Pavel Hrdina  wrote:
> > On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
> >> On 25 May 2018 at 16:20, Pavel Hrdina  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 :/
> >> >>
> >> >> > GLiblibvirt
> >> >> >
> >> >> > 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

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

2018-05-29 Thread Sukrit Bhatnagar 
On 29 May 2018 at 10:36, Michal Privoznik  wrote:
> On 05/28/2018 09:34 AM, Sukrit Bhatnagar wrote:
>>
>> 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);
>
> As Erik pointed out, this last VIR_FREE(table) looks fishy. However, do
> you have patch that I can apply and reproduce?

When I was using the above function, even without the last VIR_FREE(table),
I got the same error.
I am now using the original virArpTableFree function, not this.

The error is resolved for now. It seems like some other files I modified were
creating trouble. I reset them.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-28 Thread Michal Privoznik 
On 05/28/2018 09:34 AM, Sukrit Bhatnagar wrote:
> 
> 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);

As Erik pointed out, this last VIR_FREE(table) looks fishy. However, do
you have patch that I can apply and reproduce?

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-28 Thread Sukrit Bhatnagar 
On 28 May 2018 at 13:54, Pavel Hrdina  wrote:
> On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
>> On 25 May 2018 at 16:20, Pavel Hrdina  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 :/
>> >>
>> >> > GLiblibvirt
>> >> >
>> >> > 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

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

2018-05-28 Thread Pavel Hrdina 
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
> On 25 May 2018 at 16:20, Pavel Hrdina  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 :/
> >>
> >> > GLiblibvirt
> >> >
> >> > 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.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-28 Thread Erik Skultety 
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
> On 25 May 2018 at 16:20, Pavel Hrdina  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 :/
> >>
> >> > GLiblibvirt
> >> >
> >> > 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);


Hmm, so gcc claims the cleanup attribute to take a function which is required
to take a pointer to the type which it already is, it's just typedef'd, I
haven't tried this myself but do you get a compiler error if you try it with
the existing function, i.e. the one which doesn't mention the explicit '*' to
signal a pointer type? If so and we can't use the our typedef'd Ptr type
directly, then we'll need to switch all the free callback signatures to a plain
virSomeType *arg rather than trying to pass a double pointer which is just
unnecessary and ugly.

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

My honest guess would be ^this you're trying to free the double pointer itself
which you didn't and you're also not supposed to allocate.

 Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-28 Thread Sukrit Bhatnagar 
On 25 May 2018 at 16:20, Pavel Hrdina  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 :/
>>
>> > GLiblibvirt
>> >
>> > 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);
}

but I am getting the errors:
*** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-virbuftest':
free(): invalid pointer: 0x7ffc7be60d48 ***
...
*** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-commandtest':
free(): invalid pointer: 0x7fff727583fc ***
...

I am not quite sure how to debug this. Am I missing something basic?

>
> Note: don't take the types and function names as something that actually
> exists and be used like that, it's just an example to show how it would
> work :).
>
> Pavel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Pavel Hrdina 
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 :/
> 
> > GLiblibvirt
> > 
> > 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);

Note: don't take the types and function names as something that actually
exists and be used like that, it's just an example to show how it would
work :).

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Andrea Bolognani 
On Fri, 2018-05-25 at 11:17 +0200, Erik Skultety wrote:
> I also like the alternative approach Andrea mentioned, especially since you
> can reuse it for structures using plain scalar types, e.g. a structure like
> 
> typedef _virSomething virSomething;
> typedef virSomething * virSomethingPtr;
> struct _virSomething {
> int a;
> long b;
> bool c;
> };
> 
> ..where you don't really need to use VIR_AUTOPTR's complex clean functions
> since a simple virFree works just fine and it lets you to stay consistent with
> the usage of XPtr types which has also been mentioned already, just my 2c.

I have to disagree here: I think every virSomething should have
a corresponding virSomethingFree() function, even when it ends up
doing nothing but calling VIR_FREE() or virObjectUnref().

Even when automatic cleaning will be in place, there will still
be cases in which you have to allocate an object and return it
for the caller to free, and not having to look up what specific
free function needs to be used because you can rely on the
existence of a consistently named one is a big plus in my book.

We're pretty bad at this already, let's not make it worse :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Andrea Bolognani 
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 :/

> GLiblibvirt
> 
> 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.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Erik Skultety 
On Fri, May 25, 2018 at 11:03:01AM +0200, Andrea Bolognani wrote:
> On Fri, 2018-05-25 at 10:46 +0200, Pavel Hrdina wrote:
> > On Fri, May 25, 2018 at 10:32:04AM +0200, Andrea Bolognani wrote:
> > > I'm probably missing something, but couldn't you just have
> > >
> > >   #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> > >
> > > which you would then use as
> > >
> > > VIR_AUTOFREE(char *) string = NULL;
> > >
> > > instead?
> >
> > Yes you can have that as well, but it doesn't look ugly to you? :)
>
> Quite the opposite - not only it's consistent with the other
> macros, but it also cleanly separates the type from the variable
> name, which I consider a plus.

I also like the alternative approach Andrea mentioned, especially since you
can reuse it for structures using plain scalar types, e.g. a structure like

typedef _virSomething virSomething;
typedef virSomething * virSomethingPtr;
struct _virSomething {
int a;
long b;
bool c;
};

..where you don't really need to use VIR_AUTOPTR's complex clean functions
since a simple virFree works just fine and it lets you to stay consistent with
the usage of XPtr types which has also been mentioned already, just my 2c.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Andrea Bolognani 
On Fri, 2018-05-25 at 10:46 +0200, Pavel Hrdina wrote:
> On Fri, May 25, 2018 at 10:32:04AM +0200, Andrea Bolognani wrote:
> > I'm probably missing something, but couldn't you just have
> > 
> >   #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> > 
> > which you would then use as
> > 
> > VIR_AUTOFREE(char *) string = NULL;
> > 
> > instead?
> 
> Yes you can have that as well, but it doesn't look ugly to you? :)

Quite the opposite - not only it's consistent with the other
macros, but it also cleanly separates the type from the variable
name, which I consider a plus.

Personally, even though

  char *string;

and friends are the accepted way to declare pointer variables,
I've always been slightly annoyed by the fact that type name and
variable name end up being partially mixed together.

Don't get me wrong, something like

  char* string;

would still look wrong to me, because I'm just so used to the
other way! But in the context of that macro I think we really get
the best of both worlds :)

> If majority will agree on that I don't care, just my opinion.

Yeah, same here.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Pavel Hrdina 
On Fri, May 25, 2018 at 10:32:04AM +0200, Andrea Bolognani wrote:
> On Fri, 2018-05-25 at 10:08 +0200, Pavel Hrdina wrote:
> > On Fri, May 25, 2018 at 09:01:03AM +0200, Andrea Bolognani wrote:
> > > On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
> > > > On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
> > > 
> > > [...]
> > > > > VIR_AUTOFREE char *str = NULL;
> > > > 
> > > > For consistency I'd prefer if the argument is in parentheses similarly
> > > > to the ones below.
> > > 
> > > Seconded.
> > 
> > Well that would mean having this macro:
> > 
> > #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type *
> > 
> > and the usage would be:
> > 
> > VIR_AUTOFREE(char) string = NULL;
> > 
> > Yes, for consistency it make sense but sometimes exception makes it look
> > better and IMHO this is the case so I would prefer
> > 
> > #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree)))
> > 
> > and
> > 
> > VIR_AUTOFREE char *string = NULL;
> 
> I'm probably missing something, but couldn't you just have
> 
>   #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> 
> which you would then use as
> 
> VIR_AUTOFREE(char *) string = NULL;
> 
> instead?

Yes you can have that as well, but it doesn't look ugly to you? :)
If majority will agree on that I don't care, just my opinion.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Andrea Bolognani 
On Fri, 2018-05-25 at 10:08 +0200, Pavel Hrdina wrote:
> On Fri, May 25, 2018 at 09:01:03AM +0200, Andrea Bolognani wrote:
> > On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
> > > On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
> > 
> > [...]
> > > > VIR_AUTOFREE char *str = NULL;
> > > 
> > > For consistency I'd prefer if the argument is in parentheses similarly
> > > to the ones below.
> > 
> > Seconded.
> 
> Well that would mean having this macro:
> 
> #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type *
> 
> and the usage would be:
> 
> VIR_AUTOFREE(char) string = NULL;
> 
> Yes, for consistency it make sense but sometimes exception makes it look
> better and IMHO this is the case so I would prefer
> 
> #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree)))
> 
> and
> 
> VIR_AUTOFREE char *string = NULL;

I'm probably missing something, but couldn't you just have

  #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type

which you would then use as

VIR_AUTOFREE(char *) string = NULL;

instead?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Pavel Hrdina 
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
> On Wed, 2018-05-23 at 18:05 +0200, Pavel Hrdina wrote:
> > 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
> > 
> > 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);
> 
> I assume you would get a compilation error when trying to eg. use
> VIR_AUTOCLEAR() with a type that doesn't have a clear function
> registered?

Yes, because the macro would call non existing function.

> As for VIR_AUTOFREE() and VIR_AUTOPTR(), I'd very much prefer if we
> could have a single macro, since from the high-level point of view
> they're both doing the same thing, that is, freeing memory that was
> allocated on the heap.

It would be nice but I don't think it's worth it.

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

GLiblibvirt

g_autofree  VIR_AUTOFREE
g_autoptr   VIR_AUTOPTR
g_auto  VIR_AUTOCLEAR


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.

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.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Pavel Hrdina 
On Fri, May 25, 2018 at 09:01:03AM +0200, Andrea Bolognani wrote:
> On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
> > On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
> [...]
> > > VIR_AUTOFREE char *str = NULL;
> > 
> > For consistency I'd prefer if the argument is in parentheses similarly
> > to the ones below.
> 
> Seconded.

Well that would mean having this macro:

#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type *

and the usage would be:

VIR_AUTOFREE(char) string = NULL;

Yes, for consistency it make sense but sometimes exception makes it look
better and IMHO this is the case so I would prefer

#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree)))

and

VIR_AUTOFREE char *string = NULL;


The only sensible usage would be to have only VIR_AUTOPTR as you've
mentioned in other mail, but that would require a lot of typedefs.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Andrea Bolognani 
On Wed, 2018-05-23 at 18:05 +0200, Pavel Hrdina wrote:
> 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
> 
> 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);

I assume you would get a compilation error when trying to eg. use
VIR_AUTOCLEAR() with a type that doesn't have a clear function
registered?

As for VIR_AUTOFREE() and VIR_AUTOPTR(), I'd very much prefer if we
could have a single macro, since from the high-level point of view
they're both doing the same thing, that is, freeing memory that was
allocated on the heap.

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?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-25 Thread Andrea Bolognani 
On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
> On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
[...]
> > VIR_AUTOFREE char *str = NULL;
> 
> For consistency I'd prefer if the argument is in parentheses similarly
> to the ones below.

Seconded.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-24 Thread Pavel Hrdina 
On Wed, May 23, 2018 at 06:23:01PM +0200, Peter Krempa wrote:
> On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
> 
> [...]
> 
> > 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;
> 
> For consistency I'd prefer if the argument is in parentheses similarly
> to the ones below.

Yes, for consistency it can take the type as an argument but it's not
necessary since the type is not used by that macro.

> > - this would call virFree on that variable
> > 
> > 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);
> 
> Did you mean virDomainPtr?

Right, it can be virDomainPtr since we already have these typedefs,
GLib implementation actually creates the same typedef inside the
G_DEFINE_AUTOPTR_CLEANUP_FUNC macro, we don't have to do it.

It also creates a wrapper function with a name based on the passed type
and that wrapper function is then used by g_autoptr() again based on the
passed type.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-24 Thread Pavel Hrdina 
On Thu, May 24, 2018 at 11:16:40PM +0530, Sukrit Bhatnagar wrote:
> On 23 May 2018 at 21:35, Pavel Hrdina  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)

...

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

2018-05-24 Thread Sukrit Bhatnagar 
On 23 May 2018 at 21:35, Pavel Hrdina  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].
>
>

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

2018-05-23 Thread Peter Krempa 
On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:

[...]

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

For consistency I'd prefer if the argument is in parentheses similarly
to the ones below.

> 
> - this would call virFree on that variable
> 
> 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);

Did you mean virDomainPtr?

How is the type matched? Does it have to be a typedef'd type to use
this?



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-23 Thread Pavel Hrdina 
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

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] 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-03-26 Thread Daniel P . Berrangé 
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.

Note we call VIR_FORCE_CLOSE and VIR_FORCE_FCLOSE in cleanup scenarios
to explicitly ignore the return value. We only care about checking
return value for VIR_CLOSE and VIR_FCLOSE in a tiny set of cases.

> 
> - 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.*

There shouldn't be any need to touch the DIspose funtions - that's internal
helper code. To "free" an object variable, you just want to end up invoking
the regular virObjectUnref function as the cleanup func.

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

Just suffix it with "Auto", eg   virDomainAutoPtr, vs virDomainPtr.

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

I'm not rally seeing a compelling need for this - just add it in appropriate
existing files.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-03-24 Thread Sukrit Bhatnagar 
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.


Your suggestions are welcome.


Thanks,
Sukrit Bhatnagar
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list