[libvirt] [RFC 1/4] add macros for implementing automatic cleanup functionality

[Pavel Hrdina]

On Thu, May 31, 2018 at 02:50:31PM +0200, Erik Skultety wrote:
> On Wed, May 30, 2018 at 02:30:04AM +0530, Sukrit Bhatnagar wrote:
> > New macros are added to src/util/viralloc.h which help in
> > adding cleanup attribute to variable declarations.
> >
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr at gmail.com>
> > ---
> >  src/util/viralloc.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >
> > diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> > index 69d0f90..e1c31c3 100644
> > --- a/src/util/viralloc.h
> > +++ b/src/util/viralloc.h
> > @@ -596,4 +596,73 @@ void virAllocTestInit(void);
> >  int virAllocTestCount(void);
> >  void virAllocTestOOM(int n, int m);
> >  void virAllocTestHook(void (*func)(int, void*), void *data);
> > +
> > +# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> > +# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
> > +
> > +/**
> > + * VIR_DEFINE_AUTOPTR_FUNC:
> > + * @type: type of the variables(s) to free automatically
> > + * @func: cleanup function to be automatically called
> > + *
> > + * Thos macro defines a function for automatically freeing
> 
> s/Thos/This
> s/automatically freeing resources/automatic freeing of resources
> 
> > + * resources allocated to a variable of type @type. The newly
> > + * defined function calls the corresponding pre-defined
> > + * function @func.
> > + */
> > +# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > +    static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
> > +    { \
> > +        (func)(*_ptr); \
> > +    } \
> > +
> > +/**
> > + * VIR_DEFINE_AUTOCLEAR_FUNC:
> > + * @type: type of the variables(s) to free automatically
> > + * @func: cleanup function to be automatically called
> > + *
> > + * This macro defines a function for automatically clearing
> 
> same minor nit as above...
> 
> > + * a variable of type @type. The newly deifined function calls
> > + * the corresponding pre-defined function @func.
> > + */
> > +# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > +    static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > +    { \
> > +        (func)(_ptr); \
> > +    } \
> > +
> > +/**
> > + * VIR_AUTOFREE:
> > + * @type: type of the variables(s) to free automatically
> > + *
> > + * Macro to automatically free the memory allocated to
> > + * the variable(s) declared with it by calling virFree
> > + * when the variable goes out of scope.
> > + */
> > +# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> > +
> > +/**
> > + * VIR_AUTOPTR:
> > + * @type: type of the variables(s) to free automatically
> > + *
> > + * Macro to automatically free the memory allocated to
> > + * the variable(s) declared with it by calling the function
> > + * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
> > + * goes out of scope.
> > + */
> > +# define VIR_AUTOPTR(type) \
> > +    __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
> > +
> > +/**
> > + * VIR_AUTOCLEAR:
> > + * @type: type of the variables(s) to free automatically
> > + *
> > + * Macro to automatically clear the variable(s) declared
> > + * with it by calling the function defined by
> > + * VIR_DEFINE_AUTOCLEAR_FUNC when the variabl* goes out
> 
> s/*/e
> 
> > + * of scope.
> > + */
> > +# define VIR_AUTOCLEAR(type) \
> > +    __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
> 
> I don't see any functional problems here, I like the changes, however, if we go
> ahead with merging any future patch sets, I'd probably like to see them to
> follow the steps we discussed earlier (what the initial focus should be) since,
> subjectively, it poses a better logical order:
> 
> 1 patch series
> 1) Introduce VIR_AUTOFREE macro
> 2) use it at as many modules and places as possible (doesn't need to 100%,
> since there probably be a few caveats)
> 
> another patch series
> 3) Introduce VIR_AUTOPTR and friends
> 4) use those in as many places as the simple straightforward replacement goes
> 
> another patch series
> 5) Introduce VIR_AUTOCLEAR and friends
> 6) use those

I don't share the same view, this would result in three huge patch
series and it would be annoying to review it.

IMHO introducing all of these in a single patch make sense and after
that we should follow with incremental switch to these macros ideally
having one patch per file.  Some parts of the existing code would need
some modification before we can use these macros which would be done
separately from the one patch that implements the usage of these macros.

Switching majority of libvirt code into this new concept is dangerous
since it may introduce some issues or bugs and having small changes
makes it easier to figure out what cause the issue.

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/20180531/2572b386/attachment-0001.sig>