1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v2 0/4] Get rid of VIR_AUTOPTR(virString)
  4. View patch

[libvirt] [PATCH v2 4/4] util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors

Peter Krempa posted 4 patches 6 years, 11 months ago
[libvirt] [PATCH v2 4/4] util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors
Posted by Peter Krempa 6 years, 11 months ago 
We'd free only the first element of the vector leaking the rest.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/util/viralloc.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index 15451d4673..572b7d1c1c 100644
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -650,6 +650,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data);
  * the variable declared with it by calling the function
  * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
  * goes out of scope.
+ *
+ * Note that this macro must NOT be used with vectors! The cleaning function
+ * will not free any elements beyond the first.
  */
 # define VIR_AUTOPTR(type) \
     __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type *
@@ -662,6 +665,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data);
  * when the variable goes out of scope.
  * The cleanup function is registered by VIR_DEFINE_AUTOCLEAN_FUNC macro for
  * the given type.
+ *
+ * Note that this macro must NOT be used with vectors! The cleaning function
+ * will not free any elements beyond the first.
  */
 # define VIR_AUTOCLEAN(type) \
     __attribute__((cleanup(VIR_AUTOCLEAN_FUNC_NAME(type)))) type
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/4] util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors
Posted by Erik Skultety 6 years, 11 months ago 
On Tue, Feb 26, 2019 at 04:48:26PM +0100, Peter Krempa wrote:
> We'd free only the first element of the vector leaking the rest.
>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/util/viralloc.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> index 15451d4673..572b7d1c1c 100644
> --- a/src/util/viralloc.h
> +++ b/src/util/viralloc.h
> @@ -650,6 +650,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data);
>   * the variable declared with it by calling the function
>   * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
>   * goes out of scope.
> + *
> + * Note that this macro must NOT be used with vectors! The cleaning function
> + * will not free any elements beyond the first.

s/cleaning/freeing/

I understand, but if you have happen to have a dedicated list type, then you'd
have a dedicated destructor, so both of these would be okay with vectors. On
the other hand I'm not sure whether we have such a thing at the moment, so I
guess it's meaningful to document in the meantime.

Reviewed-by: Erik Skultety <eskultet@redhat.com>

>   */
>  # define VIR_AUTOPTR(type) \
>      __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type *
> @@ -662,6 +665,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data);
>   * when the variable goes out of scope.
>   * The cleanup function is registered by VIR_DEFINE_AUTOCLEAN_FUNC macro for
>   * the given type.
> + *
> + * Note that this macro must NOT be used with vectors! The cleaning function
> + * will not free any elements beyond the first.
>   */
>  # define VIR_AUTOCLEAN(type) \
>      __attribute__((cleanup(VIR_AUTOCLEAN_FUNC_NAME(type)))) type
> --
> 2.20.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
Re: [libvirt] [PATCH v2 4/4] util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors
Posted by Peter Krempa 6 years, 11 months ago 
On Wed, Feb 27, 2019 at 15:05:18 +0100, Erik Skultety wrote:
> On Tue, Feb 26, 2019 at 04:48:26PM +0100, Peter Krempa wrote:
> > We'd free only the first element of the vector leaking the rest.
> >
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/util/viralloc.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> > index 15451d4673..572b7d1c1c 100644
> > --- a/src/util/viralloc.h
> > +++ b/src/util/viralloc.h
> > @@ -650,6 +650,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data);
> >   * the variable declared with it by calling the function
> >   * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
> >   * goes out of scope.
> > + *
> > + * Note that this macro must NOT be used with vectors! The cleaning function
> > + * will not free any elements beyond the first.
> 
> s/cleaning/freeing/
> 
> I understand, but if you have happen to have a dedicated list type, then you'd
> have a dedicated destructor, so both of these would be okay with vectors. On

Note that the function registered via __attribute(cleanup ... gets only
the pointer to the stack'd variable as an argument. This means that you
can do only 'value-terminated' (NULL, -1, ... ) lists.

Anything requiring count of elements will need to be encapsulated in a
struct which makes it a container. Thus the comment does not apply.

> the other hand I'm not sure whether we have such a thing at the moment, so I
> guess it's meaningful to document in the meantime.

I don't think we'll get much value terminated lists because the usage is
quite cumbersome and error-prone (e.g. if you forget your terminator).

In such case we can always do a rather simple macro for them.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/4] util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors
Posted by Erik Skultety 6 years, 11 months ago 
On Thu, Feb 28, 2019 at 02:56:36PM +0100, Peter Krempa wrote:
> On Wed, Feb 27, 2019 at 15:05:18 +0100, Erik Skultety wrote:
> > On Tue, Feb 26, 2019 at 04:48:26PM +0100, Peter Krempa wrote:
> > > We'd free only the first element of the vector leaking the rest.
> > >
> > > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > > ---
> > >  src/util/viralloc.h | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> > > index 15451d4673..572b7d1c1c 100644
> > > --- a/src/util/viralloc.h
> > > +++ b/src/util/viralloc.h
> > > @@ -650,6 +650,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data);
> > >   * the variable declared with it by calling the function
> > >   * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
> > >   * goes out of scope.
> > > + *
> > > + * Note that this macro must NOT be used with vectors! The cleaning function
> > > + * will not free any elements beyond the first.
> >
> > s/cleaning/freeing/
> >
> > I understand, but if you have happen to have a dedicated list type, then you'd
> > have a dedicated destructor, so both of these would be okay with vectors. On
>
> Note that the function registered via __attribute(cleanup ... gets only
> the pointer to the stack'd variable as an argument. This means that you
> can do only 'value-terminated' (NULL, -1, ... ) lists.
>
> Anything requiring count of elements will need to be encapsulated in a
> struct which makes it a container. Thus the comment does not apply.

Yeah, true.

Erik

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.