New macros are added to src/util/viralloc.h which help in
adding cleanup attribute to variable declarations.

Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@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..bb37c48 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
+ *
+ * This macro defines a function for automatic freeing of
+ * 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 automatic clearing of
+ * 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 variable goes out
+ * of scope.
+ */
+# define VIR_AUTOCLEAR(type) \
+    __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
+
 #endif /* __VIR_MEMORY_H_ */
-- 
1.8.3.1

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

On Sun, Jun 03, 2018 at 01:41:59PM +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@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..bb37c48 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

- we are only going to use this with a single variable (or object if you will),
  so 'variables' doesn't sound right, how about:

"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. The newly
> + * defined function calls the corresponding pre-defined

"This newly defined function works as a necessary wrapper around @func"

> + * 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 automatic clearing of
> + * a variable of type @type. The newly deifined function calls
> + * the corresponding pre-defined function @func.

Same comments as above...


> + */
> +# 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

here too...

> + *
> + * Macro to automatically free the memory allocated to
> + * the variable(s) declared with it by calling virFree

s/(s)//

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

same here...

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

and here...

> + * with it by calling the function defined by
> + * VIR_DEFINE_AUTOCLEAR_FUNC when the variable goes out
> + * of scope.
> + */
> +# define VIR_AUTOCLEAR(type) \
> +    __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
> +
>  #endif /* __VIR_MEMORY_H_ */
> --
> 1.8.3.1

Sorry, I missed these in the RFC, now that I read it again, I thought of a few
wording improvements, functionally though, still fine and you have my R-b.

Erik

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

On Sun, Jun 03, 2018 at 01:41:59PM +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@gmail.com>

If you recall the recent RFC thread [1], I agreed with Pavel that we should
introduce complete changes to each file, so that we don't need to track was has
been already done for what file and what is still missing, so the series would
look something like this:

1) vir<some_file>.c preparations (especially VIR_{AUTOPTR,AUTOCLEAR} might need
that)
2) Use VIR_AUTOFREE across vir<some_file>.c
3) Use VIR_AUTOPTR across vir<some_file>.c
4) Use VIR_AUTOCLEAR across vir<some_file>.c

...One more thing, the commit subject of every patch in the current series
should probably look like this:
util: <module>: Use VIR_AUTOFREE instead of VIR_FREE for scalar types

and the corresponding commit message:
By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE
macro, majority of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

[1] https://www.redhat.com/archives/libvir-list/2018-May/msg02391.html

Erik

> ---
>  src/util/viralloc.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
>
> diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> index 69d0f90..bb37c48 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
> + *
> + * This macro defines a function for automatic freeing of
> + * 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 automatic clearing of
> + * 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 variable goes out
> + * of scope.
> + */
> +# define VIR_AUTOCLEAR(type) \
> +    __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
> +
>  #endif /* __VIR_MEMORY_H_ */
> --
> 1.8.3.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

On Tue, 5 Jun 2018 at 21:00, Erik Skultety <eskultet@redhat.com> wrote:
>
> On Sun, Jun 03, 2018 at 01:41:59PM +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@gmail.com>
>
> If you recall the recent RFC thread [1], I agreed with Pavel that we should
> introduce complete changes to each file, so that we don't need to track was has
> been already done for what file and what is still missing, so the series would
> look something like this:
>
> 1) vir<some_file>.c preparations (especially VIR_{AUTOPTR,AUTOCLEAR} might need
> that)
> 2) Use VIR_AUTOFREE across vir<some_file>.c
> 3) Use VIR_AUTOPTR across vir<some_file>.c
> 4) Use VIR_AUTOCLEAR across vir<some_file>.c
>
> ...One more thing, the commit subject of every patch in the current series
> should probably look like this:
> util: <module>: Use VIR_AUTOFREE instead of VIR_FREE for scalar types
>
> and the corresponding commit message:
> By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE
> macro, majority of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> [1] https://www.redhat.com/archives/libvir-list/2018-May/msg02391.html

I have some rudimentary queries regarding this approach:

- The <module> in commit subject should be like "vircgroup" or just "cgroup"?
- Do I create a separate series of patch even for those files who do not require
  VIR_AUTOPTR and VIR_AUTOCLEAR but only VIR_AUTOFREE,
  like viraudit.c?
- Do I use the same commit subject and message, with minor changes of course,
  for all patches in a series (AUTOFREE, AUTOPTR and AUTOCLEAR)?
- In the case of files like virauth.c which require changes in
virauthconfig.h instead
  of virauth.h, do I batch all the files virauth* together?

I am assuming that the changes to header file vir<some_file>.h
corresponding to vir<some_file>.c will be in the same series if needed.

--
Thanks,
Sukrit

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

On Wed, Jun 06, 2018 at 06:42:29PM +0530, Sukrit Bhatnagar wrote:
> On Tue, 5 Jun 2018 at 21:00, Erik Skultety <eskultet@redhat.com> wrote:
> >
> > On Sun, Jun 03, 2018 at 01:41:59PM +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@gmail.com>
> >
> > If you recall the recent RFC thread [1], I agreed with Pavel that we should
> > introduce complete changes to each file, so that we don't need to track was has
> > been already done for what file and what is still missing, so the series would
> > look something like this:
> >
> > 1) vir<some_file>.c preparations (especially VIR_{AUTOPTR,AUTOCLEAR} might need
> > that)
> > 2) Use VIR_AUTOFREE across vir<some_file>.c
> > 3) Use VIR_AUTOPTR across vir<some_file>.c
> > 4) Use VIR_AUTOCLEAR across vir<some_file>.c
> >
> > ...One more thing, the commit subject of every patch in the current series
> > should probably look like this:
> > util: <module>: Use VIR_AUTOFREE instead of VIR_FREE for scalar types
> >
> > and the corresponding commit message:
> > By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE
> > macro, majority of the VIR_FREE calls can be dropped, which in turn leads to
> > getting rid of most of our cleanup sections.
> >
> > [1] https://www.redhat.com/archives/libvir-list/2018-May/msg02391.html
>
> I have some rudimentary queries regarding this approach:
>
> - The <module> in commit subject should be like "vircgroup" or just "cgroup"?

preferably "cgroup" (you'll save 3 chars :)), but doesn't really matter, I
think I pushed a patch yesterday where I actually added the 'vir' prefix, so
whatever...

> - Do I create a separate series of patch even for those files who do not require
>   VIR_AUTOPTR and VIR_AUTOCLEAR but only VIR_AUTOFREE,
>   like viraudit.c?

I'm getting the impression we're not on the same page yet, I suppose I should
have added something like "..." in the example above or simply be more clear in
general.

So, let's be more specific, this series was 18 patches long and you need to add
2 more patches per file, so that would be 54 patches in a single series, that's
big, so how about taking 5-10 files instead of 18, that would produce roughly
15-30 patches in a single series (a bit more manageable and as I mentioned
somewhere else, complete changes that are fine can be merged independently) -
so it's not going to be a separate series per file, rather a single series,
following the pattern I mentioned in my previous response above, performing
changes to up to N selected files, does it make more sense now?

> - Do I use the same commit subject and message, with minor changes of course,
>   for all patches in a series (AUTOFREE, AUTOPTR and AUTOCLEAR)?

Yep.

> - In the case of files like virauth.c which require changes in
> virauthconfig.h instead
>   of virauth.h, do I batch all the files virauth* together?

Yes, well, it should be coupled based on the change you're making, but you just
mentioned it - "required changes" - IOW the patch would not otherwise compile.

>
> I am assuming that the changes to header file vir<some_file>.h
> corresponding to vir<some_file>.c will be in the same series if needed.

See above, but yes.

Let me know if there's still something which is not clear.
Erik

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

On Wed, 6 Jun 2018 at 21:25, Erik Skultety <eskultet@redhat.com> wrote:
>
> On Wed, Jun 06, 2018 at 06:42:29PM +0530, Sukrit Bhatnagar wrote:
> > On Tue, 5 Jun 2018 at 21:00, Erik Skultety <eskultet@redhat.com> wrote:
> > >
> > > On Sun, Jun 03, 2018 at 01:41:59PM +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@gmail.com>
> > >
> > > If you recall the recent RFC thread [1], I agreed with Pavel that we should
> > > introduce complete changes to each file, so that we don't need to track was has
> > > been already done for what file and what is still missing, so the series would
> > > look something like this:
> > >
> > > 1) vir<some_file>.c preparations (especially VIR_{AUTOPTR,AUTOCLEAR} might need
> > > that)
> > > 2) Use VIR_AUTOFREE across vir<some_file>.c
> > > 3) Use VIR_AUTOPTR across vir<some_file>.c
> > > 4) Use VIR_AUTOCLEAR across vir<some_file>.c
> > >
> > > ...One more thing, the commit subject of every patch in the current series
> > > should probably look like this:
> > > util: <module>: Use VIR_AUTOFREE instead of VIR_FREE for scalar types
> > >
> > > and the corresponding commit message:
> > > By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE
> > > macro, majority of the VIR_FREE calls can be dropped, which in turn leads to
> > > getting rid of most of our cleanup sections.
> > >
> > > [1] https://www.redhat.com/archives/libvir-list/2018-May/msg02391.html
> >
> > I have some rudimentary queries regarding this approach:
> >
> > - The <module> in commit subject should be like "vircgroup" or just "cgroup"?
>
> preferably "cgroup" (you'll save 3 chars :)), but doesn't really matter, I
> think I pushed a patch yesterday where I actually added the 'vir' prefix, so
> whatever...
>
> > - Do I create a separate series of patch even for those files who do not require
> >   VIR_AUTOPTR and VIR_AUTOCLEAR but only VIR_AUTOFREE,
> >   like viraudit.c?
>
> I'm getting the impression we're not on the same page yet, I suppose I should
> have added something like "..." in the example above or simply be more clear in
> general.
>
> So, let's be more specific, this series was 18 patches long and you need to add
> 2 more patches per file, so that would be 54 patches in a single series, that's
> big, so how about taking 5-10 files instead of 18, that would produce roughly
> 15-30 patches in a single series (a bit more manageable and as I mentioned
> somewhere else, complete changes that are fine can be merged independently) -
> so it's not going to be a separate series per file, rather a single series,
> following the pattern I mentioned in my previous response above, performing
> changes to up to N selected files, does it make more sense now?

Yes, it does now.

> > - Do I use the same commit subject and message, with minor changes of course,
> >   for all patches in a series (AUTOFREE, AUTOPTR and AUTOCLEAR)?
>
> Yep.
>
> > - In the case of files like virauth.c which require changes in
> > virauthconfig.h instead
> >   of virauth.h, do I batch all the files virauth* together?
>
> Yes, well, it should be coupled based on the change you're making, but you just
> mentioned it - "required changes" - IOW the patch would not otherwise compile.
>
> >
> > I am assuming that the changes to header file vir<some_file>.h
> > corresponding to vir<some_file>.c will be in the same series if needed.
>
> See above, but yes.
>
> Let me know if there's still something which is not clear.

No other queries for now, thanks!

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