Re: [libvirt] [PATCH v3 01/35] util: alloc: add macros for implementing automatic cleanup functionality

On Thu, 12 Jul 2018 at 22:09, Erik Skultety <eskultet(a)redhat.com&gt; wrote: > > On Sat, Jun 30, 2018 at 02:30:05PM +0530, Sukrit Bhatnagar wrote: > > New macros are introduced which help in adding GNU C's cleanup > > attribute to variable declarations. Variables declared with these > > macros will have their allocated memory freed automatically when > > they go out of scope. > > > > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com&gt; > > --- > > src/util/viralloc.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/src/util/viralloc.h b/src/util/viralloc.h > > index 69d0f90..5c1d0d5 100644 > > --- a/src/util/viralloc.h > > +++ b/src/util/viralloc.h > > @@ -596,4 +596,48 @@ void virAllocTestInit(void); > > int virAllocTestCount(void); > > void virAllocTestOOM(int n, int m); > > void virAllocTestHook(void (*func)(int, void*), void *data); > > + > > +# define VIR_AUTOPTR_TYPE_NAME(type) type##AutoPtr > > +# define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree > > + > > +/** > > + * VIR_DEFINE_AUTOPTR_FUNC: > > + * @type: type of the variable to be freed automatically > > + * @func: cleanup function to be automatically called > > + * > > + * This macro defines a function for automatic freeing of > > + * resources allocated to a variable of type @type. This newly > > + * defined function works as a necessary wrapper around @func. > > + */ > > +# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \ > > + typedef type *VIR_AUTOPTR_TYPE_NAME(type); \ > > So, it's not visible at first glance how ^this typedef is used... > > > + static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \ > > + { \ > > + if (*_ptr) \ > > + (func)(*_ptr); \ > > + *_ptr = NULL; \ > > + } \ > > ...therefore I'd write it explicitly as > > VIR_AUTOPTR_FUNC_NAME(type)(VIR_AUTOPTR_TYPE_NAME(type) *_ptr) > > > + > > +# define VIR_AUTOPTR(type) \ > > + __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) VIR_AUTOPTR_TYPE_NAME(type) > > + > > Also, since we're going to use it like this: > VIR_AUTOPTR(virDomainDef) foo > > instead of this: > VIR_AUTOPTR(virDomainDefPtr) foo > > why do we need VIR_AUTOPTR_TYPE_NAME anyway, since you could just use > "type *" in the VIR_AUTOPTR macro definition and that should work for external > types as well. I agree. We don't really need it. Here is how the code will look after the changes you suggested: # define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \ static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \ { \ if (*_ptr) \ (func)(*_ptr); \ *_ptr = NULL; \ } \ # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type # define VIR_AUTOPTR(type) \ __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type * Shall I proceed to send the first series of patches?