[libvirt] [RFC 1/4] add macros for implementing automatic cleanup functionality
Erik Skultety
eskultet at redhat.com
Thu May 31 13:37:25 UTC 2018
On Thu, May 31, 2018 at 03:12:17PM +0200, Pavel Hrdina wrote:
> 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.
Yeah, I elaborated on my vision more in the last patch, I understand that it's
not visible immediately from this response, my bad, this was just supposed to
be a general idea, we'll of course need some granularity, noone wants to review
such a beast.
>
> 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.
>
After a private discussion with Pavel, an alternative approach (again, using
some degree of granularity) would be to perform all the necessary changes for
each module of the series (e.g. a third of the the modules within src/util per
series - this is just an idea) in separate patches of the same series (1 patch
for VIR_AUTOFREE, 1 for AUTOPTR, 1 for AUTOCLEAR, possibly more in order to
prepare the module for such changes), thus knowing for sure which files we've
converted already and therefore not keeping track of what changes have been to
which modules. I have to give it to Pavel, that this seems like a better
approach given the timeline for GSoC.
> 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.
It sure is, without arguments...
Erik
More information about the libvir-list
mailing list