11:36 a.m.
Hi,
I am starting this discussion thread as a continuation of my GSoC
weekly meeting with Erik and Pavel on 8th June.
I was going through src/util/virstring.c for adding cleanup macros and
saw that virStringListFree takes on char ** as an argument, and
equivalently, we declare a list of strings as char **.
For the cleanup function defined by VIR_DEFINE_AUTOPTR_FUNC, it is
required that the associated type has a name like virSomethingPtr.
It was also discussed that there are similar issues with DBus types,
but VIR_AUTOFREE can work there as we use VIR_ALLOC. I honestly don't
know much about that.
We discussed that we have two solutions:
- Create a virSomethingPtr by typedef-ing char**
As Pavel told, GLib has typedef gchar** GStrv; which is used together
with g_auto and it has g_strfreev(gchar **str_array) which is the same
as we have virStringListFree()
I have tried adding the following in src/util/virstrnig.h, and it
seems to work fine:
typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
We can use it as:
VIR_AUTOPTR(virStringList) lines = NULL;
There may be other scalar and external types where this problem
occurs, and it is not good to create a typedef for each of them, but
maybe we can make an exception for char ** and create a type for it.
- Overload VIR_AUTOFREE macro by making it variadic
As Erik told, we could make VIR_AUTOFREE a variadic macro whose
varying parameter can be the Free function name. If left blank, we use
virFree.
I went ahead with trying it and after reading some posts on
StackOverflow, I came up with this:
#define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
#define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
#define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
#define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
_VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
The required functionality is working as expected; passing only one
argument will use virFree, and passing two arguments will use the
function specified as 2nd argument. Passing more than 2 arguments will
result in an error.
The macros with _ prefix are meant to be for internal use only.
Also, @func needs to be a wrapper around virStringListFree as it will
take char ***, not just char **. We probably need to define a new
function.
Here we are specifying the Free function to use at the time of usage
of the VIR_AUTOFREE macro, which may make the code look bad:
VIR_AUTOFREE(char **, virStringListSomethingFree) lines = NULL;
Suggestions and opinions are welcome.
Thanks,
Sukrit
4:12 p.m.
On Sat, Jun 09, 2018 at 10:06:55PM +0530, Sukrit Bhatnagar wrote:
Hi,
I am starting this discussion thread as a continuation of my GSoC
weekly meeting with Erik and Pavel on 8th June.
I was going through src/util/virstring.c for adding cleanup macros and
saw that virStringListFree takes on char ** as an argument, and
equivalently, we declare a list of strings as char **.
For the cleanup function defined by VIR_DEFINE_AUTOPTR_FUNC, it is
required that the associated type has a name like virSomethingPtr.
It was also discussed that there are similar issues with DBus types,
but VIR_AUTOFREE can work there as we use VIR_ALLOC. I honestly don't
know much about that.
We discussed that we have two solutions:
- Create a virSomethingPtr by typedef-ing char**
As Pavel told, GLib has typedef gchar** GStrv; which is used together
with g_auto and it has g_strfreev(gchar **str_array) which is the same
as we have virStringListFree()
I have tried adding the following in src/util/virstrnig.h, and it
seems to work fine:
typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
We can use it as:
VIR_AUTOPTR(virStringList) lines = NULL;
There may be other scalar and external types where this problem
occurs, and it is not good to create a typedef for each of them, but
maybe we can make an exception for char ** and create a type for it.
- Overload VIR_AUTOFREE macro by making it variadic
As Erik told, we could make VIR_AUTOFREE a variadic macro whose
varying parameter can be the Free function name. If left blank, we use
virFree.
I went ahead with trying it and after reading some posts on
StackOverflow, I came up with this:
#define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
#define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
#define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
#define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
_VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
The required functionality is working as expected; passing only one
argument will use virFree, and passing two arguments will use the
function specified as 2nd argument. Passing more than 2 arguments will
result in an error.
The macros with _ prefix are meant to be for internal use only.
Also, @func needs to be a wrapper around virStringListFree as it will
take char ***, not just char **. We probably need to define a new
function.
Here we are specifying the Free function to use at the time of usage
of the VIR_AUTOFREE macro, which may make the code look bad:
VIR_AUTOFREE(char **, virStringListSomethingFree) lines = NULL;
Suggestions and opinions are welcome.
Just my two cents, but I like the second variant more. For few reasons:
- If we typedef char ** to something, then all users of that function will need
to cast it back to char ** since they will be accessing the underlying strings
(char *), even if there is a macro or a function for that, it seems the code
will be less readable.
- We are using a trick similar to the second variant in tests/virmock.h,
although for a different purpose. See VIR_MOCK_COUNT_ARGS
- With the first approach we're going to have to create unnecessary types and
possibly lot of them.
Have a nice day,
Martin
Thanks,
Sukrit
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:12 a.m.
On Sat, Jun 09, 2018 at 11:12:29PM +0200, Martin Kletzander wrote:
On Sat, Jun 09, 2018 at 10:06:55PM +0530, Sukrit Bhatnagar wrote:
> Hi,
>
> I am starting this discussion thread as a continuation of my GSoC
> weekly meeting with Erik and Pavel on 8th June.
>
> I was going through src/util/virstring.c for adding cleanup macros and
> saw that virStringListFree takes on char ** as an argument, and
> equivalently, we declare a list of strings as char **.
>
> For the cleanup function defined by VIR_DEFINE_AUTOPTR_FUNC, it is
> required that the associated type has a name like virSomethingPtr.
>
> It was also discussed that there are similar issues with DBus types,
> but VIR_AUTOFREE can work there as we use VIR_ALLOC. I honestly don't
> know much about that.
>
> We discussed that we have two solutions:
>
> - Create a virSomethingPtr by typedef-ing char**
>
> As Pavel told, GLib has typedef gchar** GStrv; which is used together
> with g_auto and it has g_strfreev(gchar **str_array) which is the same
> as we have virStringListFree()
>
> I have tried adding the following in src/util/virstrnig.h, and it
> seems to work fine:
>
> typedef char **virStringList;
> VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
>
> We can use it as:
> VIR_AUTOPTR(virStringList) lines = NULL;
>
> There may be other scalar and external types where this problem
> occurs, and it is not good to create a typedef for each of them, but
> maybe we can make an exception for char ** and create a type for it.
>
>
> - Overload VIR_AUTOFREE macro by making it variadic
>
> As Erik told, we could make VIR_AUTOFREE a variadic macro whose
> varying parameter can be the Free function name. If left blank, we use
> virFree.
>
> I went ahead with trying it and after reading some posts on
> StackOverflow, I came up with this:
>
> #define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
> #define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
>
> #define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
> #define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
> _VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
>
> The required functionality is working as expected; passing only one
> argument will use virFree, and passing two arguments will use the
> function specified as 2nd argument. Passing more than 2 arguments will
> result in an error.
>
> The macros with _ prefix are meant to be for internal use only.
> Also, @func needs to be a wrapper around virStringListFree as it will
> take char ***, not just char **. We probably need to define a new
> function.
>
> Here we are specifying the Free function to use at the time of usage
> of the VIR_AUTOFREE macro, which may make the code look bad:
> VIR_AUTOFREE(char **, virStringListSomethingFree) lines = NULL;
>
>
> Suggestions and opinions are welcome.
I don't like this solution for several reasons, first of all
VIR_AUTOFREE should be as simple as calling virFree(). If we decide for
this or similar solution, we should create a new macro, not overload
this one.
In order to use this we would have to create another free function for
each specific type anyway because the function passed to attribute
cleanup takes a pointer to the actual type so as Sukrit mentioned
virStringListFree would not be good enough and there would have to be
some wrapper for it.
Just my two cents, but I like the second variant more. For few
reasons:
- If we typedef char ** to something, then all users of that function will need
to cast it back to char ** since they will be accessing the underlying strings
(char *), even if there is a macro or a function for that, it seems the code
will be less readable.
- We are using a trick similar to the second variant in tests/virmock.h,
although for a different purpose. See VIR_MOCK_COUNT_ARGS
- With the first approach we're going to have to create unnecessary types and
possibly lot of them.
Yes, for each type we would have to create a new typePtr and that is not
nice so we might need to revise the design of VIR_AUTOPTR to take 'type'
instead of 'typePtr' and as GLib is doing and create the special typedef
only inside the macro and only for this purpose.
Another issue that we need to take into account is that the external
free functions might not be 'NULL' safe which we need to somehow ensure
if they will be used with attribute cleanup.
The original design is probably wrong and was heavily counting on the
existing libvirt implementation. We might need to update the design
to make it the similar as GLib:
#define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
#define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
#define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) VIR_AUTOPTR_TYPE_NAME(type)
#define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
{ \
if (*_ptr) \
(func)(*_ptr); \
}
The usage would like this:
VIR_DEFINE_AUTOPTR_FUNC(virAuthConfig, virAuthConfigFree)
VIR_AUTOPTR(virAuthConfig) authConfig = NULL;
That way we can use any existing free function regardless whether it
checks if the variable is NULL or not and without the need to manually
define new type.
The only exception would be the virStringList where we would have to use
the different type.
Pavel
5:43 a.m.
On Mon, Jun 11, 2018 at 12:12:16PM +0200, Pavel Hrdina wrote:
On Sat, Jun 09, 2018 at 11:12:29PM +0200, Martin Kletzander wrote:
> On Sat, Jun 09, 2018 at 10:06:55PM +0530, Sukrit Bhatnagar wrote:
> > Hi,
> >
> > I am starting this discussion thread as a continuation of my GSoC
> > weekly meeting with Erik and Pavel on 8th June.
> >
> > I was going through src/util/virstring.c for adding cleanup macros and
> > saw that virStringListFree takes on char ** as an argument, and
> > equivalently, we declare a list of strings as char **.
> >
> > For the cleanup function defined by VIR_DEFINE_AUTOPTR_FUNC, it is
> > required that the associated type has a name like virSomethingPtr.
> >
> > It was also discussed that there are similar issues with DBus types,
> > but VIR_AUTOFREE can work there as we use VIR_ALLOC. I honestly don't
> > know much about that.
> >
> > We discussed that we have two solutions:
> >
> > - Create a virSomethingPtr by typedef-ing char**
> >
> > As Pavel told, GLib has typedef gchar** GStrv; which is used together
> > with g_auto and it has g_strfreev(gchar **str_array) which is the same
> > as we have virStringListFree()
> >
> > I have tried adding the following in src/util/virstrnig.h, and it
> > seems to work fine:
> >
> > typedef char **virStringList;
> > VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
> >
> > We can use it as:
> > VIR_AUTOPTR(virStringList) lines = NULL;
> >
> > There may be other scalar and external types where this problem
> > occurs, and it is not good to create a typedef for each of them, but
> > maybe we can make an exception for char ** and create a type for it.
> >
> >
> > - Overload VIR_AUTOFREE macro by making it variadic
> >
> > As Erik told, we could make VIR_AUTOFREE a variadic macro whose
> > varying parameter can be the Free function name. If left blank, we use
> > virFree.
> >
> > I went ahead with trying it and after reading some posts on
> > StackOverflow, I came up with this:
> >
> > #define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
> > #define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
> >
> > #define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
> > #define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
> > _VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
> >
> > The required functionality is working as expected; passing only one
> > argument will use virFree, and passing two arguments will use the
> > function specified as 2nd argument. Passing more than 2 arguments will
> > result in an error.
> >
> > The macros with _ prefix are meant to be for internal use only.
> > Also, @func needs to be a wrapper around virStringListFree as it will
> > take char ***, not just char **. We probably need to define a new
> > function.
> >
> > Here we are specifying the Free function to use at the time of usage
> > of the VIR_AUTOFREE macro, which may make the code look bad:
> > VIR_AUTOFREE(char **, virStringListSomethingFree) lines = NULL;
> >
> >
> > Suggestions and opinions are welcome.
I don't like this solution for several reasons, first of all
VIR_AUTOFREE should be as simple as calling virFree(). If we decide for
this or similar solution, we should create a new macro, not overload
this one.
In order to use this we would have to create another free function for
each specific type anyway because the function passed to attribute
cleanup takes a pointer to the actual type so as Sukrit mentioned
virStringListFree would not be good enough and there would have to be
some wrapper for it.
> Just my two cents, but I like the second variant more. For few reasons:
>
> - If we typedef char ** to something, then all users of that function will need
> to cast it back to char ** since they will be accessing the underlying strings
> (char *), even if there is a macro or a function for that, it seems the code
> will be less readable.
>
> - We are using a trick similar to the second variant in tests/virmock.h,
> although for a different purpose. See VIR_MOCK_COUNT_ARGS
>
> - With the first approach we're going to have to create unnecessary types and
> possibly lot of them.
Yes, for each type we would have to create a new typePtr and that is not
nice so we might need to revise the design of VIR_AUTOPTR to take 'type'
instead of 'typePtr' and as GLib is doing and create the special typedef
only inside the macro and only for this purpose.
Another issue that we need to take into account is that the external
free functions might not be 'NULL' safe which we need to somehow ensure
if they will be used with attribute cleanup.
The original design is probably wrong and was heavily counting on the
existing libvirt implementation. We might need to update the design
to make it the similar as GLib:
#define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
#define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
#define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) VIR_AUTOPTR_TYPE_NAME(type)
#define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
{ \
if (*_ptr) \
(func)(*_ptr); \
}
I haven't followed all the discussions, but won't this be a problem
Sukrit is talking about that we need to fix? For `char **` the above
will just not work.
Just making sure we all understand each other.
5:53 a.m.
On Mon, Jun 11, 2018 at 12:43:59PM +0200, Martin Kletzander wrote:
On Mon, Jun 11, 2018 at 12:12:16PM +0200, Pavel Hrdina wrote:
> On Sat, Jun 09, 2018 at 11:12:29PM +0200, Martin Kletzander wrote:
> > On Sat, Jun 09, 2018 at 10:06:55PM +0530, Sukrit Bhatnagar wrote:
> > > Hi,
> > >
> > > I am starting this discussion thread as a continuation of my GSoC
> > > weekly meeting with Erik and Pavel on 8th June.
> > >
> > > I was going through src/util/virstring.c for adding cleanup macros and
> > > saw that virStringListFree takes on char ** as an argument, and
> > > equivalently, we declare a list of strings as char **.
> > >
> > > For the cleanup function defined by VIR_DEFINE_AUTOPTR_FUNC, it is
> > > required that the associated type has a name like virSomethingPtr.
> > >
> > > It was also discussed that there are similar issues with DBus types,
> > > but VIR_AUTOFREE can work there as we use VIR_ALLOC. I honestly don't
> > > know much about that.
> > >
> > > We discussed that we have two solutions:
> > >
> > > - Create a virSomethingPtr by typedef-ing char**
> > >
> > > As Pavel told, GLib has typedef gchar** GStrv; which is used together
> > > with g_auto and it has g_strfreev(gchar **str_array) which is the same
> > > as we have virStringListFree()
> > >
> > > I have tried adding the following in src/util/virstrnig.h, and it
> > > seems to work fine:
> > >
> > > typedef char **virStringList;
> > > VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
> > >
> > > We can use it as:
> > > VIR_AUTOPTR(virStringList) lines = NULL;
> > >
> > > There may be other scalar and external types where this problem
> > > occurs, and it is not good to create a typedef for each of them, but
> > > maybe we can make an exception for char ** and create a type for it.
> > >
> > >
> > > - Overload VIR_AUTOFREE macro by making it variadic
> > >
> > > As Erik told, we could make VIR_AUTOFREE a variadic macro whose
> > > varying parameter can be the Free function name. If left blank, we use
> > > virFree.
> > >
> > > I went ahead with trying it and after reading some posts on
> > > StackOverflow, I came up with this:
> > >
> > > #define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
> > > #define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
> > >
> > > #define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
> > > #define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
> > > _VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
> > >
> > > The required functionality is working as expected; passing only one
> > > argument will use virFree, and passing two arguments will use the
> > > function specified as 2nd argument. Passing more than 2 arguments will
> > > result in an error.
> > >
> > > The macros with _ prefix are meant to be for internal use only.
> > > Also, @func needs to be a wrapper around virStringListFree as it will
> > > take char ***, not just char **. We probably need to define a new
> > > function.
> > >
> > > Here we are specifying the Free function to use at the time of usage
> > > of the VIR_AUTOFREE macro, which may make the code look bad:
> > > VIR_AUTOFREE(char **, virStringListSomethingFree) lines = NULL;
> > >
> > >
> > > Suggestions and opinions are welcome.
>
> I don't like this solution for several reasons, first of all
> VIR_AUTOFREE should be as simple as calling virFree(). If we decide for
> this or similar solution, we should create a new macro, not overload
> this one.
>
> In order to use this we would have to create another free function for
> each specific type anyway because the function passed to attribute
> cleanup takes a pointer to the actual type so as Sukrit mentioned
> virStringListFree would not be good enough and there would have to be
> some wrapper for it.
>
> > Just my two cents, but I like the second variant more. For few reasons:
> >
> > - If we typedef char ** to something, then all users of that function will
need
> > to cast it back to char ** since they will be accessing the underlying
strings
> > (char *), even if there is a macro or a function for that, it seems the code
> > will be less readable.
> >
> > - We are using a trick similar to the second variant in tests/virmock.h,
> > although for a different purpose. See VIR_MOCK_COUNT_ARGS
> >
> > - With the first approach we're going to have to create unnecessary types
and
> > possibly lot of them.
>
> Yes, for each type we would have to create a new typePtr and that is not
> nice so we might need to revise the design of VIR_AUTOPTR to take 'type'
> instead of 'typePtr' and as GLib is doing and create the special typedef
> only inside the macro and only for this purpose.
>
> Another issue that we need to take into account is that the external
> free functions might not be 'NULL' safe which we need to somehow ensure
> if they will be used with attribute cleanup.
>
> The original design is probably wrong and was heavily counting on the
> existing libvirt implementation. We might need to update the design
> to make it the similar as GLib:
>
> #define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> #define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
>
> #define VIR_AUTOPTR(type) \
> __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
>
> #define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> { \
> if (*_ptr) \
> (func)(*_ptr); \
> }
>
I haven't followed all the discussions, but won't this be a problem
Sukrit is talking about that we need to fix? For `char **` the above
will just not work.
Just making sure we all understand each other.
In the very last sentence of Pavel's last reply he said that char ** would have
to be special-cased regardless ;).
Erik
5:59 a.m.
On Mon, Jun 11, 2018 at 12:53:47PM +0200, Erik Skultety wrote:
On Mon, Jun 11, 2018 at 12:43:59PM +0200, Martin Kletzander wrote:
> On Mon, Jun 11, 2018 at 12:12:16PM +0200, Pavel Hrdina wrote:
> > On Sat, Jun 09, 2018 at 11:12:29PM +0200, Martin Kletzander wrote:
> > > On Sat, Jun 09, 2018 at 10:06:55PM +0530, Sukrit Bhatnagar wrote:
> > > > Hi,
> > > >
> > > > I am starting this discussion thread as a continuation of my GSoC
> > > > weekly meeting with Erik and Pavel on 8th June.
> > > >
> > > > I was going through src/util/virstring.c for adding cleanup macros
and
> > > > saw that virStringListFree takes on char ** as an argument, and
> > > > equivalently, we declare a list of strings as char **.
> > > >
> > > > For the cleanup function defined by VIR_DEFINE_AUTOPTR_FUNC, it is
> > > > required that the associated type has a name like virSomethingPtr.
> > > >
> > > > It was also discussed that there are similar issues with DBus types,
> > > > but VIR_AUTOFREE can work there as we use VIR_ALLOC. I honestly
don't
> > > > know much about that.
> > > >
> > > > We discussed that we have two solutions:
> > > >
> > > > - Create a virSomethingPtr by typedef-ing char**
> > > >
> > > > As Pavel told, GLib has typedef gchar** GStrv; which is used together
> > > > with g_auto and it has g_strfreev(gchar **str_array) which is the
same
> > > > as we have virStringListFree()
> > > >
> > > > I have tried adding the following in src/util/virstrnig.h, and it
> > > > seems to work fine:
> > > >
> > > > typedef char **virStringList;
> > > > VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
> > > >
> > > > We can use it as:
> > > > VIR_AUTOPTR(virStringList) lines = NULL;
> > > >
> > > > There may be other scalar and external types where this problem
> > > > occurs, and it is not good to create a typedef for each of them, but
> > > > maybe we can make an exception for char ** and create a type for it.
> > > >
> > > >
> > > > - Overload VIR_AUTOFREE macro by making it variadic
> > > >
> > > > As Erik told, we could make VIR_AUTOFREE a variadic macro whose
> > > > varying parameter can be the Free function name. If left blank, we
use
> > > > virFree.
> > > >
> > > > I went ahead with trying it and after reading some posts on
> > > > StackOverflow, I came up with this:
> > > >
> > > > #define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
> > > > #define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func)))
type
> > > >
> > > > #define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
> > > > #define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
> > > > _VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
> > > >
> > > > The required functionality is working as expected; passing only one
> > > > argument will use virFree, and passing two arguments will use the
> > > > function specified as 2nd argument. Passing more than 2 arguments
will
> > > > result in an error.
> > > >
> > > > The macros with _ prefix are meant to be for internal use only.
> > > > Also, @func needs to be a wrapper around virStringListFree as it will
> > > > take char ***, not just char **. We probably need to define a new
> > > > function.
> > > >
> > > > Here we are specifying the Free function to use at the time of usage
> > > > of the VIR_AUTOFREE macro, which may make the code look bad:
> > > > VIR_AUTOFREE(char **, virStringListSomethingFree) lines = NULL;
> > > >
> > > >
> > > > Suggestions and opinions are welcome.
> >
> > I don't like this solution for several reasons, first of all
> > VIR_AUTOFREE should be as simple as calling virFree(). If we decide for
> > this or similar solution, we should create a new macro, not overload
> > this one.
> >
> > In order to use this we would have to create another free function for
> > each specific type anyway because the function passed to attribute
> > cleanup takes a pointer to the actual type so as Sukrit mentioned
> > virStringListFree would not be good enough and there would have to be
> > some wrapper for it.
> >
> > > Just my two cents, but I like the second variant more. For few reasons:
> > >
> > > - If we typedef char ** to something, then all users of that function will
need
> > > to cast it back to char ** since they will be accessing the underlying
strings
> > > (char *), even if there is a macro or a function for that, it seems the
code
> > > will be less readable.
> > >
> > > - We are using a trick similar to the second variant in tests/virmock.h,
> > > although for a different purpose. See VIR_MOCK_COUNT_ARGS
> > >
> > > - With the first approach we're going to have to create unnecessary
types and
> > > possibly lot of them.
> >
> > Yes, for each type we would have to create a new typePtr and that is not
> > nice so we might need to revise the design of VIR_AUTOPTR to take
'type'
> > instead of 'typePtr' and as GLib is doing and create the special
typedef
> > only inside the macro and only for this purpose.
> >
> > Another issue that we need to take into account is that the external
> > free functions might not be 'NULL' safe which we need to somehow ensure
> > if they will be used with attribute cleanup.
> >
> > The original design is probably wrong and was heavily counting on the
> > existing libvirt implementation. We might need to update the design
> > to make it the similar as GLib:
> >
> > #define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> > #define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
> >
> > #define VIR_AUTOPTR(type) \
> > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
> >
> > #define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
> > static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> > { \
> > if (*_ptr) \
> > (func)(*_ptr); \
> > }
> >
>
> I haven't followed all the discussions, but won't this be a problem
> Sukrit is talking about that we need to fix? For `char **` the above
> will just not work.
>
> Just making sure we all understand each other.
In the very last sentence of Pavel's last reply he said that char ** would have
to be special-cased regardless ;).
Maybe that's why I shouldn't mix in such conversations. Because I'm blind :)
Anyway if that is special-cased then we still need to make a special function
for that, so it feels contradictory to the rest of the message. With Sukrit's
variant 2 it wouldn't have to be. And it would still be as easy to use for
other types as Pavel wants it to be.
But don't get blocked on me ;)
6:05 a.m.
On Mon, Jun 11, 2018 at 12:59:11PM +0200, Martin Kletzander wrote:
On Mon, Jun 11, 2018 at 12:53:47PM +0200, Erik Skultety wrote:
> On Mon, Jun 11, 2018 at 12:43:59PM +0200, Martin Kletzander wrote:
> > On Mon, Jun 11, 2018 at 12:12:16PM +0200, Pavel Hrdina wrote:
> > > On Sat, Jun 09, 2018 at 11:12:29PM +0200, Martin Kletzander wrote:
> > > > On Sat, Jun 09, 2018 at 10:06:55PM +0530, Sukrit Bhatnagar wrote:
> > > > > Hi,
> > > > >
> > > > > I am starting this discussion thread as a continuation of my
GSoC
> > > > > weekly meeting with Erik and Pavel on 8th June.
> > > > >
> > > > > I was going through src/util/virstring.c for adding cleanup
macros and
> > > > > saw that virStringListFree takes on char ** as an argument, and
> > > > > equivalently, we declare a list of strings as char **.
> > > > >
> > > > > For the cleanup function defined by VIR_DEFINE_AUTOPTR_FUNC, it
is
> > > > > required that the associated type has a name like
virSomethingPtr.
> > > > >
> > > > > It was also discussed that there are similar issues with DBus
types,
> > > > > but VIR_AUTOFREE can work there as we use VIR_ALLOC. I honestly
don't
> > > > > know much about that.
> > > > >
> > > > > We discussed that we have two solutions:
> > > > >
> > > > > - Create a virSomethingPtr by typedef-ing char**
> > > > >
> > > > > As Pavel told, GLib has typedef gchar** GStrv; which is used
together
> > > > > with g_auto and it has g_strfreev(gchar **str_array) which is
the same
> > > > > as we have virStringListFree()
> > > > >
> > > > > I have tried adding the following in src/util/virstrnig.h, and
it
> > > > > seems to work fine:
> > > > >
> > > > > typedef char **virStringList;
> > > > > VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
> > > > >
> > > > > We can use it as:
> > > > > VIR_AUTOPTR(virStringList) lines = NULL;
> > > > >
> > > > > There may be other scalar and external types where this problem
> > > > > occurs, and it is not good to create a typedef for each of them,
but
> > > > > maybe we can make an exception for char ** and create a type for
it.
> > > > >
> > > > >
> > > > > - Overload VIR_AUTOFREE macro by making it variadic
> > > > >
> > > > > As Erik told, we could make VIR_AUTOFREE a variadic macro whose
> > > > > varying parameter can be the Free function name. If left blank,
we use
> > > > > virFree.
> > > > >
> > > > > I went ahead with trying it and after reading some posts on
> > > > > StackOverflow, I came up with this:
> > > > >
> > > > > #define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree)))
type
> > > > > #define _VIR_AUTOFREE_1(type, func)
__attribute__((cleanup(func))) type
> > > > >
> > > > > #define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
> > > > > #define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
> > > > > _VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
> > > > >
> > > > > The required functionality is working as expected; passing only
one
> > > > > argument will use virFree, and passing two arguments will use
the
> > > > > function specified as 2nd argument. Passing more than 2
arguments will
> > > > > result in an error.
> > > > >
> > > > > The macros with _ prefix are meant to be for internal use only.
> > > > > Also, @func needs to be a wrapper around virStringListFree as it
will
> > > > > take char ***, not just char **. We probably need to define a
new
> > > > > function.
> > > > >
> > > > > Here we are specifying the Free function to use at the time of
usage
> > > > > of the VIR_AUTOFREE macro, which may make the code look bad:
> > > > > VIR_AUTOFREE(char **, virStringListSomethingFree) lines = NULL;
> > > > >
> > > > >
> > > > > Suggestions and opinions are welcome.
> > >
> > > I don't like this solution for several reasons, first of all
> > > VIR_AUTOFREE should be as simple as calling virFree(). If we decide for
> > > this or similar solution, we should create a new macro, not overload
> > > this one.
> > >
> > > In order to use this we would have to create another free function for
> > > each specific type anyway because the function passed to attribute
> > > cleanup takes a pointer to the actual type so as Sukrit mentioned
> > > virStringListFree would not be good enough and there would have to be
> > > some wrapper for it.
> > >
> > > > Just my two cents, but I like the second variant more. For few
reasons:
> > > >
> > > > - If we typedef char ** to something, then all users of that function
will need
> > > > to cast it back to char ** since they will be accessing the
underlying strings
> > > > (char *), even if there is a macro or a function for that, it seems
the code
> > > > will be less readable.
> > > >
> > > > - We are using a trick similar to the second variant in
tests/virmock.h,
> > > > although for a different purpose. See VIR_MOCK_COUNT_ARGS
> > > >
> > > > - With the first approach we're going to have to create
unnecessary types and
> > > > possibly lot of them.
> > >
> > > Yes, for each type we would have to create a new typePtr and that is not
> > > nice so we might need to revise the design of VIR_AUTOPTR to take
'type'
> > > instead of 'typePtr' and as GLib is doing and create the special
typedef
> > > only inside the macro and only for this purpose.
> > >
> > > Another issue that we need to take into account is that the external
> > > free functions might not be 'NULL' safe which we need to somehow
ensure
> > > if they will be used with attribute cleanup.
> > >
> > > The original design is probably wrong and was heavily counting on the
> > > existing libvirt implementation. We might need to update the design
> > > to make it the similar as GLib:
> > >
> > > #define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> > > #define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
> > >
> > > #define VIR_AUTOPTR(type) \
> > > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
> > >
> > > #define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > > typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
> > > static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> > > { \
> > > if (*_ptr) \
> > > (func)(*_ptr); \
> > > }
> > >
> >
> > I haven't followed all the discussions, but won't this be a problem
> > Sukrit is talking about that we need to fix? For `char **` the above
> > will just not work.
> >
> > Just making sure we all understand each other.
>
> In the very last sentence of Pavel's last reply he said that char ** would have
> to be special-cased regardless ;).
>
Maybe that's why I shouldn't mix in such conversations. Because I'm blind
:)
Anyway if that is special-cased then we still need to make a special function
for that, so it feels contradictory to the rest of the message. With Sukrit's
variant 2 it wouldn't have to be. And it would still be as easy to use for
other types as Pavel wants it to be.
But don't get blocked on me ;)
We would not need special function for list of string, we would need
only special typedef:
typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
VIR_AUTOPTR(virStringList) list = NULL;
Pavel
1:29 p.m.
On Mon, 11 Jun 2018 at 16:35, Pavel Hrdina <phrdina(a)redhat.com> wrote:
On Mon, Jun 11, 2018 at 12:59:11PM +0200, Martin Kletzander wrote:
> On Mon, Jun 11, 2018 at 12:53:47PM +0200, Erik Skultety wrote:
> > On Mon, Jun 11, 2018 at 12:43:59PM +0200, Martin Kletzander wrote:
> > > On Mon, Jun 11, 2018 at 12:12:16PM +0200, Pavel Hrdina wrote:
> > > > On Sat, Jun 09, 2018 at 11:12:29PM +0200, Martin Kletzander wrote:
> > > > > On Sat, Jun 09, 2018 at 10:06:55PM +0530, Sukrit Bhatnagar
wrote:
> > > > > > Hi,
> > > > > >
> > > > > > I am starting this discussion thread as a continuation of
my GSoC
> > > > > > weekly meeting with Erik and Pavel on 8th June.
> > > > > >
> > > > > > I was going through src/util/virstring.c for adding cleanup
macros and
> > > > > > saw that virStringListFree takes on char ** as an argument,
and
> > > > > > equivalently, we declare a list of strings as char **.
> > > > > >
> > > > > > For the cleanup function defined by
VIR_DEFINE_AUTOPTR_FUNC, it is
> > > > > > required that the associated type has a name like
virSomethingPtr.
> > > > > >
> > > > > > It was also discussed that there are similar issues with
DBus types,
> > > > > > but VIR_AUTOFREE can work there as we use VIR_ALLOC. I
honestly don't
> > > > > > know much about that.
> > > > > >
> > > > > > We discussed that we have two solutions:
> > > > > >
> > > > > > - Create a virSomethingPtr by typedef-ing char**
> > > > > >
> > > > > > As Pavel told, GLib has typedef gchar** GStrv; which is
used together
> > > > > > with g_auto and it has g_strfreev(gchar **str_array) which
is the same
> > > > > > as we have virStringListFree()
> > > > > >
> > > > > > I have tried adding the following in src/util/virstrnig.h,
and it
> > > > > > seems to work fine:
> > > > > >
> > > > > > typedef char **virStringList;
> > > > > > VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
> > > > > >
> > > > > > We can use it as:
> > > > > > VIR_AUTOPTR(virStringList) lines = NULL;
> > > > > >
> > > > > > There may be other scalar and external types where this
problem
> > > > > > occurs, and it is not good to create a typedef for each of
them, but
> > > > > > maybe we can make an exception for char ** and create a
type for it.
> > > > > >
> > > > > >
> > > > > > - Overload VIR_AUTOFREE macro by making it variadic
> > > > > >
> > > > > > As Erik told, we could make VIR_AUTOFREE a variadic macro
whose
> > > > > > varying parameter can be the Free function name. If left
blank, we use
> > > > > > virFree.
> > > > > >
> > > > > > I went ahead with trying it and after reading some posts
on
> > > > > > StackOverflow, I came up with this:
> > > > > >
> > > > > > #define _VIR_AUTOFREE_0(type)
__attribute__((cleanup(virFree))) type
> > > > > > #define _VIR_AUTOFREE_1(type, func)
__attribute__((cleanup(func))) type
> > > > > >
> > > > > > #define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
> > > > > > #define VIR_AUTOFREE(...)
_VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
> > > > > > _VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
> > > > > >
> > > > > > The required functionality is working as expected; passing
only one
> > > > > > argument will use virFree, and passing two arguments will
use the
> > > > > > function specified as 2nd argument. Passing more than 2
arguments will
> > > > > > result in an error.
> > > > > >
> > > > > > The macros with _ prefix are meant to be for internal use
only.
> > > > > > Also, @func needs to be a wrapper around virStringListFree
as it will
> > > > > > take char ***, not just char **. We probably need to define
a new
> > > > > > function.
> > > > > >
> > > > > > Here we are specifying the Free function to use at the time
of usage
> > > > > > of the VIR_AUTOFREE macro, which may make the code look
bad:
> > > > > > VIR_AUTOFREE(char **, virStringListSomethingFree) lines =
NULL;
> > > > > >
> > > > > >
> > > > > > Suggestions and opinions are welcome.
> > > >
> > > > I don't like this solution for several reasons, first of all
> > > > VIR_AUTOFREE should be as simple as calling virFree(). If we decide
for
> > > > this or similar solution, we should create a new macro, not overload
> > > > this one.
> > > >
> > > > In order to use this we would have to create another free function
for
> > > > each specific type anyway because the function passed to attribute
> > > > cleanup takes a pointer to the actual type so as Sukrit mentioned
> > > > virStringListFree would not be good enough and there would have to
be
> > > > some wrapper for it.
> > > >
> > > > > Just my two cents, but I like the second variant more. For few
reasons:
> > > > >
> > > > > - If we typedef char ** to something, then all users of that
function will need
> > > > > to cast it back to char ** since they will be accessing the
underlying strings
> > > > > (char *), even if there is a macro or a function for that, it
seems the code
> > > > > will be less readable.
> > > > >
> > > > > - We are using a trick similar to the second variant in
tests/virmock.h,
> > > > > although for a different purpose. See VIR_MOCK_COUNT_ARGS
> > > > >
> > > > > - With the first approach we're going to have to create
unnecessary types and
> > > > > possibly lot of them.
> > > >
> > > > Yes, for each type we would have to create a new typePtr and that is
not
> > > > nice so we might need to revise the design of VIR_AUTOPTR to take
'type'
> > > > instead of 'typePtr' and as GLib is doing and create the
special typedef
> > > > only inside the macro and only for this purpose.
> > > >
> > > > Another issue that we need to take into account is that the external
> > > > free functions might not be 'NULL' safe which we need to
somehow ensure
> > > > if they will be used with attribute cleanup.
> > > >
> > > > The original design is probably wrong and was heavily counting on
the
> > > > existing libvirt implementation. We might need to update the design
> > > > to make it the similar as GLib:
> > > >
> > > > #define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> > > > #define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
> > > >
> > > > #define VIR_AUTOPTR(type) \
> > > > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
> > > >
> > > > #define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > > > typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
> > > > static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> > > > { \
> > > > if (*_ptr) \
> > > > (func)(*_ptr); \
> > > > }
> > > >
> > >
> > > I haven't followed all the discussions, but won't this be a
problem
> > > Sukrit is talking about that we need to fix? For `char **` the above
> > > will just not work.
> > >
> > > Just making sure we all understand each other.
> >
> > In the very last sentence of Pavel's last reply he said that char ** would
have
> > to be special-cased regardless ;).
> >
>
> Maybe that's why I shouldn't mix in such conversations. Because I'm
blind :)
>
> Anyway if that is special-cased then we still need to make a special function
> for that, so it feels contradictory to the rest of the message. With Sukrit's
> variant 2 it wouldn't have to be. And it would still be as easy to use for
> other types as Pavel wants it to be.
>
> But don't get blocked on me ;)
We would not need special function for list of string, we would need
only special typedef:
typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
VIR_AUTOPTR(virStringList) list = NULL;
So, are we all agreeing on this one i.e. modifying the original designed
macros to take `type` instead of `typePtr` as Pavel suggested?
1:20 a.m.
On Mon, Jun 11, 2018 at 11:59:06PM +0530, Sukrit Bhatnagar wrote:
On Mon, 11 Jun 2018 at 16:35, Pavel Hrdina <phrdina(a)redhat.com>
wrote:
>
> On Mon, Jun 11, 2018 at 12:59:11PM +0200, Martin Kletzander wrote:
> > On Mon, Jun 11, 2018 at 12:53:47PM +0200, Erik Skultety wrote:
> > > On Mon, Jun 11, 2018 at 12:43:59PM +0200, Martin Kletzander wrote:
> > > > On Mon, Jun 11, 2018 at 12:12:16PM +0200, Pavel Hrdina wrote:
> > > > > On Sat, Jun 09, 2018 at 11:12:29PM +0200, Martin Kletzander
wrote:
> > > > > > On Sat, Jun 09, 2018 at 10:06:55PM +0530, Sukrit Bhatnagar
wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > I am starting this discussion thread as a continuation
of my GSoC
> > > > > > > weekly meeting with Erik and Pavel on 8th June.
> > > > > > >
> > > > > > > I was going through src/util/virstring.c for adding
cleanup macros and
> > > > > > > saw that virStringListFree takes on char ** as an
argument, and
> > > > > > > equivalently, we declare a list of strings as char
**.
> > > > > > >
> > > > > > > For the cleanup function defined by
VIR_DEFINE_AUTOPTR_FUNC, it is
> > > > > > > required that the associated type has a name like
virSomethingPtr.
> > > > > > >
> > > > > > > It was also discussed that there are similar issues
with DBus types,
> > > > > > > but VIR_AUTOFREE can work there as we use VIR_ALLOC. I
honestly don't
> > > > > > > know much about that.
> > > > > > >
> > > > > > > We discussed that we have two solutions:
> > > > > > >
> > > > > > > - Create a virSomethingPtr by typedef-ing char**
> > > > > > >
> > > > > > > As Pavel told, GLib has typedef gchar** GStrv; which
is used together
> > > > > > > with g_auto and it has g_strfreev(gchar **str_array)
which is the same
> > > > > > > as we have virStringListFree()
> > > > > > >
> > > > > > > I have tried adding the following in
src/util/virstrnig.h, and it
> > > > > > > seems to work fine:
> > > > > > >
> > > > > > > typedef char **virStringList;
> > > > > > > VIR_DEFINE_AUTOPTR_FUNC(virStringList,
virStringListFree)
> > > > > > >
> > > > > > > We can use it as:
> > > > > > > VIR_AUTOPTR(virStringList) lines = NULL;
> > > > > > >
> > > > > > > There may be other scalar and external types where
this problem
> > > > > > > occurs, and it is not good to create a typedef for
each of them, but
> > > > > > > maybe we can make an exception for char ** and create
a type for it.
> > > > > > >
> > > > > > >
> > > > > > > - Overload VIR_AUTOFREE macro by making it variadic
> > > > > > >
> > > > > > > As Erik told, we could make VIR_AUTOFREE a variadic
macro whose
> > > > > > > varying parameter can be the Free function name. If
left blank, we use
> > > > > > > virFree.
> > > > > > >
> > > > > > > I went ahead with trying it and after reading some
posts on
> > > > > > > StackOverflow, I came up with this:
> > > > > > >
> > > > > > > #define _VIR_AUTOFREE_0(type)
__attribute__((cleanup(virFree))) type
> > > > > > > #define _VIR_AUTOFREE_1(type, func)
__attribute__((cleanup(func))) type
> > > > > > >
> > > > > > > #define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...)
NAME
> > > > > > > #define VIR_AUTOFREE(...)
_VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
> > > > > > > _VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
> > > > > > >
> > > > > > > The required functionality is working as expected;
passing only one
> > > > > > > argument will use virFree, and passing two arguments
will use the
> > > > > > > function specified as 2nd argument. Passing more than
2 arguments will
> > > > > > > result in an error.
> > > > > > >
> > > > > > > The macros with _ prefix are meant to be for internal
use only.
> > > > > > > Also, @func needs to be a wrapper around
virStringListFree as it will
> > > > > > > take char ***, not just char **. We probably need to
define a new
> > > > > > > function.
> > > > > > >
> > > > > > > Here we are specifying the Free function to use at the
time of usage
> > > > > > > of the VIR_AUTOFREE macro, which may make the code
look bad:
> > > > > > > VIR_AUTOFREE(char **, virStringListSomethingFree)
lines = NULL;
> > > > > > >
> > > > > > >
> > > > > > > Suggestions and opinions are welcome.
> > > > >
> > > > > I don't like this solution for several reasons, first of
all
> > > > > VIR_AUTOFREE should be as simple as calling virFree(). If we
decide for
> > > > > this or similar solution, we should create a new macro, not
overload
> > > > > this one.
> > > > >
> > > > > In order to use this we would have to create another free
function for
> > > > > each specific type anyway because the function passed to
attribute
> > > > > cleanup takes a pointer to the actual type so as Sukrit
mentioned
> > > > > virStringListFree would not be good enough and there would have
to be
> > > > > some wrapper for it.
> > > > >
> > > > > > Just my two cents, but I like the second variant more. For
few reasons:
> > > > > >
> > > > > > - If we typedef char ** to something, then all users of
that function will need
> > > > > > to cast it back to char ** since they will be accessing
the underlying strings
> > > > > > (char *), even if there is a macro or a function for that,
it seems the code
> > > > > > will be less readable.
> > > > > >
> > > > > > - We are using a trick similar to the second variant in
tests/virmock.h,
> > > > > > although for a different purpose. See
VIR_MOCK_COUNT_ARGS
> > > > > >
> > > > > > - With the first approach we're going to have to create
unnecessary types and
> > > > > > possibly lot of them.
> > > > >
> > > > > Yes, for each type we would have to create a new typePtr and
that is not
> > > > > nice so we might need to revise the design of VIR_AUTOPTR to
take 'type'
> > > > > instead of 'typePtr' and as GLib is doing and create the
special typedef
> > > > > only inside the macro and only for this purpose.
> > > > >
> > > > > Another issue that we need to take into account is that the
external
> > > > > free functions might not be 'NULL' safe which we need to
somehow ensure
> > > > > if they will be used with attribute cleanup.
> > > > >
> > > > > The original design is probably wrong and was heavily counting
on the
> > > > > existing libvirt implementation. We might need to update the
design
> > > > > to make it the similar as GLib:
> > > > >
> > > > > #define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> > > > > #define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
> > > > >
> > > > > #define VIR_AUTOPTR(type) \
> > > > > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
> > > > >
> > > > > #define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > > > > typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
> > > > > static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr)
\
> > > > > { \
> > > > > if (*_ptr) \
> > > > > (func)(*_ptr); \
> > > > > }
> > > > >
> > > >
> > > > I haven't followed all the discussions, but won't this be a
problem
> > > > Sukrit is talking about that we need to fix? For `char **` the
above
> > > > will just not work.
> > > >
> > > > Just making sure we all understand each other.
> > >
> > > In the very last sentence of Pavel's last reply he said that char **
would have
> > > to be special-cased regardless ;).
> > >
> >
> > Maybe that's why I shouldn't mix in such conversations. Because
I'm blind :)
> >
> > Anyway if that is special-cased then we still need to make a special function
> > for that, so it feels contradictory to the rest of the message. With
Sukrit's
> > variant 2 it wouldn't have to be. And it would still be as easy to use
for
> > other types as Pavel wants it to be.
> >
> > But don't get blocked on me ;)
>
> We would not need special function for list of string, we would need
> only special typedef:
>
> typedef char **virStringList;
>
> VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
>
> VIR_AUTOPTR(virStringList) list = NULL;
So, are we all agreeing on this one i.e. modifying the original designed
macros to take `type` instead of `typePtr` as Pavel suggested?
I think we might want to get a few more thoughts on this, so I put more people
on CC to speed things up a little, let's see who'll respond.
Erik
5:54 a.m.
On Mon, 2018-06-11 at 12:12 +0200, Pavel Hrdina wrote:
Another issue that we need to take into account is that the external
free functions might not be 'NULL' safe which we need to somehow ensure
if they will be used with attribute cleanup.
The original design is probably wrong and was heavily counting on the
existing libvirt implementation. We might need to update the design
to make it the similar as GLib:
#define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
#define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
#define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) VIR_AUTOPTR_TYPE_NAME(type)
#define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
{ \
if (*_ptr) \
(func)(*_ptr); \
}
The usage would like this:
VIR_DEFINE_AUTOPTR_FUNC(virAuthConfig, virAuthConfigFree)
VIR_AUTOPTR(virAuthConfig) authConfig = NULL;
That way we can use any existing free function regardless whether it
checks if the variable is NULL or not and without the need to manually
define new type.
The only exception would be the virStringList where we would have to use
the different type.
As danpb (CC'd) mentioned in [1], having the vir*Free() function
take a double pointer is good because it allows it to also set it
to NULL. I will add that any vir*Free() function that doesn't deal
gracefully with being passed a NULL pointer is already buggy and
should be fixed.
This new proposal would not deal with either, and it seems to me
like it would be preferable to stick with the original one and, as
a prerequisite, change all vir*Free() functions so that they follow
the design outlined above, as doing so would benefit all code that
still needs to perform manual memory management as well.
Are there any known issues that would make such an approach not
feasible? Would it expand the scope of the project too much?
[1] https://www.redhat.com/archives/libvir-list/2018-June/msg00033.html
--
Andrea Bolognani / Red Hat / Virtualization
5:59 a.m.
On Wed, Jun 13, 2018 at 12:54:54PM +0200, Andrea Bolognani wrote:
On Mon, 2018-06-11 at 12:12 +0200, Pavel Hrdina wrote:
> Another issue that we need to take into account is that the external
> free functions might not be 'NULL' safe which we need to somehow ensure
> if they will be used with attribute cleanup.
>
> The original design is probably wrong and was heavily counting on the
> existing libvirt implementation. We might need to update the design
> to make it the similar as GLib:
>
> #define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> #define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
>
> #define VIR_AUTOPTR(type) \
> __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
>
> #define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> { \
> if (*_ptr) \
> (func)(*_ptr); \
> }
>
> The usage would like this:
>
> VIR_DEFINE_AUTOPTR_FUNC(virAuthConfig, virAuthConfigFree)
>
> VIR_AUTOPTR(virAuthConfig) authConfig = NULL;
>
>
> That way we can use any existing free function regardless whether it
> checks if the variable is NULL or not and without the need to manually
> define new type.
>
> The only exception would be the virStringList where we would have to use
> the different type.
As danpb (CC'd) mentioned in [1], having the vir*Free() function
take a double pointer is good because it allows it to also set it
to NULL. I will add that any vir*Free() function that doesn't deal
gracefully with being passed a NULL pointer is already buggy and
should be fixed.
This new proposal would not deal with either, and it seems to me
like it would be preferable to stick with the original one and, as
a prerequisite, change all vir*Free() functions so that they follow
the design outlined above, as doing so would benefit all code that
still needs to perform manual memory management as well.
Are there any known issues that would make such an approach not
feasible? Would it expand the scope of the project too much?
You've missed the whole reason for this new proposal. Yes, we can fix
our internal vir*Free() functions but this autofree functionality would
be nice also for types from external libraries where the types have
their own free function and that we cannot fix.
Pavel
6:28 a.m.
On Wed, 2018-06-13 at 12:59 +0200, Pavel Hrdina wrote:
On Wed, Jun 13, 2018 at 12:54:54PM +0200, Andrea Bolognani wrote:
> On Mon, 2018-06-11 at 12:12 +0200, Pavel Hrdina wrote:
> > That way we can use any existing free function regardless whether it
> > checks if the variable is NULL or not and without the need to manually
> > define new type.
> >
> > The only exception would be the virStringList where we would have to use
> > the different type.
>
> As danpb (CC'd) mentioned in [1], having the vir*Free() function
> take a double pointer is good because it allows it to also set it
> to NULL. I will add that any vir*Free() function that doesn't deal
> gracefully with being passed a NULL pointer is already buggy and
> should be fixed.
>
> This new proposal would not deal with either, and it seems to me
> like it would be preferable to stick with the original one and, as
> a prerequisite, change all vir*Free() functions so that they follow
> the design outlined above, as doing so would benefit all code that
> still needs to perform manual memory management as well.
>
> Are there any known issues that would make such an approach not
> feasible? Would it expand the scope of the project too much?
You've missed the whole reason for this new proposal. Yes, we can fix
our internal vir*Free() functions but this autofree functionality would
be nice also for types from external libraries where the types have
their own free function and that we cannot fix.
Fair enough.
Should we have this *in addition* to the "libvirt native" mechanism
that was initially proposed, then? I'm not personally sure whether
that would ultimately lead to a better result, just throwing it out
there for discussion :)
--
Andrea Bolognani / Red Hat / Virtualization
6:08 a.m.
On Wed, Jun 13, 2018 at 12:54:54PM +0200, Andrea Bolognani wrote:
On Mon, 2018-06-11 at 12:12 +0200, Pavel Hrdina wrote:
> Another issue that we need to take into account is that the external
> free functions might not be 'NULL' safe which we need to somehow ensure
> if they will be used with attribute cleanup.
>
> The original design is probably wrong and was heavily counting on the
> existing libvirt implementation. We might need to update the design
> to make it the similar as GLib:
>
> #define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> #define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
>
> #define VIR_AUTOPTR(type) \
> __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
>
> #define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> { \
> if (*_ptr) \
> (func)(*_ptr); \
> }
>
> The usage would like this:
>
> VIR_DEFINE_AUTOPTR_FUNC(virAuthConfig, virAuthConfigFree)
>
> VIR_AUTOPTR(virAuthConfig) authConfig = NULL;
>
>
> That way we can use any existing free function regardless whether it
> checks if the variable is NULL or not and without the need to manually
> define new type.
>
> The only exception would be the virStringList where we would have to use
> the different type.
As danpb (CC'd) mentioned in [1], having the vir*Free() function
take a double pointer is good because it allows it to also set it
to NULL. I will add that any vir*Free() function that doesn't deal
gracefully with being passed a NULL pointer is already buggy and
should be fixed.
Where we have vir*Free() functions that do not take a double
pointer, I think we should definitely update them. Double
free is one of the more common C memory allocation mistakes,
hence why we designed our free() replacement to avoid it.
All this refactoring to use auto-pointers, while beneficial
in the long term, definitely has a short term risk of introducing
memory allocation bugs, either by double-frees, or mistakenly
auto-free'ing a pointer we intended to keep hold of. Anything
we can do to reduce these two risks is very beneficial.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
3:24 a.m.
On Sat, Jun 09, 2018 at 10:06:55PM +0530, Sukrit Bhatnagar wrote:
Hi,
I am starting this discussion thread as a continuation of my GSoC
weekly meeting with Erik and Pavel on 8th June.
I was going through src/util/virstring.c for adding cleanup macros and
saw that virStringListFree takes on char ** as an argument, and
equivalently, we declare a list of strings as char **.
For the cleanup function defined by VIR_DEFINE_AUTOPTR_FUNC, it is
required that the associated type has a name like virSomethingPtr.
It was also discussed that there are similar issues with DBus types,
but VIR_AUTOFREE can work there as we use VIR_ALLOC. I honestly don't
know much about that.
We discussed that we have two solutions:
- Create a virSomethingPtr by typedef-ing char**
As Pavel told, GLib has typedef gchar** GStrv; which is used together
with g_auto and it has g_strfreev(gchar **str_array) which is the same
as we have virStringListFree()
I have tried adding the following in src/util/virstrnig.h, and it
seems to work fine:
typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
We can use it as:
VIR_AUTOPTR(virStringList) lines = NULL;
There may be other scalar and external types where this problem
occurs, and it is not good to create a typedef for each of them, but
maybe we can make an exception for char ** and create a type for it.
- Overload VIR_AUTOFREE macro by making it variadic
As Erik told, we could make VIR_AUTOFREE a variadic macro whose
varying parameter can be the Free function name. If left blank, we use
virFree.
I went ahead with trying it and after reading some posts on
StackOverflow, I came up with this:
#define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
#define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
#define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
#define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
_VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
The required functionality is working as expected; passing only one
argument will use virFree, and passing two arguments will use the
function specified as 2nd argument. Passing more than 2 arguments will
result in an error.
So, I had something slightly different from ^this in mind, but it turns out, I
was wrong about that and your approach to what I proposed is the correct
one, a bit cryptic, but that would be the case anyway, were my idea even
possible to implement, so nevermind.
Note: if the overall consensus will be in favour of the second approach, we'll
need to work on the helper macro naming though.
The macros with _ prefix are meant to be for internal use only.
Also, @func needs to be a wrapper around virStringListFree as it will
take char ***, not just char **. We probably need to define a new
function.
Not necessarily, you could create a static inline wrapper the same way as do for
the VIR_AUTOPTR macro. But I understand Pavel's concerns, that theoretically we
can ditch everything we've come up so far and have a universal
VIR_AUTOFREE/VIR_AUTOPTR for everything (with the __VA_ARGS__ bits included),
but you already know I feel the same way about this as Martin apparently does in
that we could end up with many more typedefs than we can currently think of
using the first approach, whereas here, yeah the macro is kinda hacky, but that
doesn't matter for the user of the macro (+ we've got more hacky ones) and
whether someone passes a custom function instead of a dedicated "XFree" wrapper
or not is the responsibility of the reviewer to point out as it's with many
other things, so we shouldn't even talk about a potential misuse of the macro
itself.
Erik
5:24 a.m.
Hi,
It took me longer to sit down and write this mail but here it goes.
There was a lot of ideas about the macro design and it's easy to
get lost in all the designs.
So we agreed on this form:
# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
{ \
(func)(*_ptr); \
}
# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
{ \
(func)(_ptr); \
}
# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
# define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
# define VIR_AUTOCLEAR(type) \
__attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
but it turned out the it's not sufficient for several reasons:
- there is no simple way ho to free list of strings (char **)
- this will not work for external types with their own free function
In order to solve these two issues there were some ideas.
1. How to handle list of strings
We have virStringListFree() function in order to free list of strings,
the question is how to use it with these macros:
- Create a new typedef for list of strings and use VIR_AUTOPTR:
typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
VIR_AUTOPTR(virStringList) list = NULL;
The benefit of this solution is that it works with current design
and we only need to create single typedef.
- Overload VIR_AUTOFREE macro by making it variadic
# define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
# define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
# define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
# define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__, _VIR_AUTOFREE_1,
_VIR_AUTOFREE_0)(__VA_ARGS__)
This would allow us to use any existing function directly with
attribute cleanup. However, the function must take as argument
pointer to the specified type so we would have to create a wrapper
functions which would be used in the VIR_AUTOFREE macro, for
example:
void virStringListPtrFree(char ***stringsp)
{
virStringListFree(*stringsp);
}
VIR_AUTOFREE(char **, virStringListPtrFree) list = NULL;
2. External types with free function
Libvirt uses a lot of external libraries where we use the types and
relevant free functions. The issue with original design is that it was
counting on the fact that we would have vir*Ptr typedefs, but that's not
the case for external types.
- We can modify the design to be closer to GLib design which would
work even with external types and would be safe in case external
free functions will not behave correctly. These are to
modification to the original design:
# define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
typedef type *VIR_AUTOPTR_TYPE_NAME(type);
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
{ \
if (*_ptr) { \
(func)(*_ptr); \
*_ptr = NULL; \
} \
}
# define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) VIR_AUTOPTR_TYPE_NAME(type)
The usage would not require our internal vir*Ptr types and would
be easy to use with external types as well:
VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
VIR_AUTOPTR(virBitmap) map = NULL;
VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
There was one suggestion to make another set of macros for the
external types or just ignore them because there are only few
places which uses external free functions.
Pavel
5:52 a.m.
On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
Hi,
It took me longer to sit down and write this mail but here it goes.
There was a lot of ideas about the macro design and it's easy to
get lost in all the designs.
So we agreed on this form:
# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
{ \
(func)(*_ptr); \
}
# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
{ \
(func)(_ptr); \
}
For anything where we define the impl of (func) these two macros
should never be used IMHO. We should just change the signature of
the real functions so that accept a double pointer straight away,
and thus not require the wrapper function. Yes, it this will
require updating existing code, but such a design change is
beneficial even without the cleanup macros, as it prevents the
possbility of double frees. I'd suggest we just do this kind of
change straightaway
# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
I don't see the point in passing "type" in here we could simplify
this:
#define VIR_AUTOFREE __attribute__((cleanup(virFree))
VIR_AUTOFREE char *foo = NULL;
and at the same time fix the char ** problems
#define VIR_AUTOFREE_FUNC(func) __attribute__((cleanup(func))
VIR_AUTOFREE_FUNC(virStringListFree) char *foo = NULL;
or if we want to simplify it
#define VIR_AUTOFREE_STRINGLIST VIR_AUTOFREE_FUNC(virStringListFree)
VIR_AUTOFREE_STRINGLIST char **strs = NULL;
This also works for external libraries
# define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
# define VIR_AUTOCLEAR(type) \
__attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
but it turned out the it's not sufficient for several reasons:
- there is no simple way ho to free list of strings (char **)
- this will not work for external types with their own free function
In order to solve these two issues there were some ideas.
1. How to handle list of strings
We have virStringListFree() function in order to free list of strings,
the question is how to use it with these macros:
- Create a new typedef for list of strings and use VIR_AUTOPTR:
typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
VIR_AUTOPTR(virStringList) list = NULL;
I don't think we should be creating artifical new typedefs just to
deal with the flawed design of our own autofree macros.
- Overload VIR_AUTOFREE macro by making it variadic
# define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
# define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
# define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
# define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__, _VIR_AUTOFREE_1,
_VIR_AUTOFREE_0)(__VA_ARGS__)
This is uneccessarily black magic - just have VIR_AUTOFREE and
VIR_AUTOFREE_FUNC defined separately.
void virStringListPtrFree(char ***stringsp)
{
virStringListFree(*stringsp);
}
As above, we should just fixed virStringListFree to have the better
signature straight away.
VIR_AUTOFREE(char **, virStringListPtrFree) list = NULL;
2. External types with free function
Libvirt uses a lot of external libraries where we use the types and
relevant free functions. The issue with original design is that it was
counting on the fact that we would have vir*Ptr typedefs, but that's not
the case for external types.
- We can modify the design to be closer to GLib design which would
work even with external types and would be safe in case external
free functions will not behave correctly. These are to
modification to the original design:
# define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
typedef type *VIR_AUTOPTR_TYPE_NAME(type);
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
{ \
if (*_ptr) { \
(func)(*_ptr); \
*_ptr = NULL; \
} \
}
# define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) VIR_AUTOPTR_TYPE_NAME(type)
The usage would not require our internal vir*Ptr types and would
be easy to use with external types as well:
VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
VIR_AUTOPTR(virBitmap) map = NULL;
VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
this is a reasonable usage, because we don't control the signature
of the libssh2_channel_free function.
VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
With my example above
#define VIR_AUTOFREE_LIBSSH_CHANNEL VIR_AUTOFREE_FUNC(LIBSSH2_CHANNEL)
VIR_AUTOFREE_LIBSSH_CHANNEL LIBSSH_CHANNEL *chan = NULL;
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
6:50 a.m.
On Mon, 2018-06-18 at 11:52 +0100, Daniel P. Berrangé wrote:
On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> { \
> (func)(_ptr); \
> }
For anything where we define the impl of (func) these two macros
should never be used IMHO. We should just change the signature of
the real functions so that accept a double pointer straight away,
and thus not require the wrapper function. Yes, it this will
require updating existing code, but such a design change is
beneficial even without the cleanup macros, as it prevents the
possbility of double frees. I'd suggest we just do this kind of
change straightaway
Agreed that we want to fix our own free functions.
> # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree)))
type
I don't see the point in passing "type" in here we could simplify
this:
#define VIR_AUTOFREE __attribute__((cleanup(virFree))
VIR_AUTOFREE char *foo = NULL;
Passing the type was suggested earlier to make usage consistent
across all cases, eg.
VIR_AUTOFREE(char *) str = NULL;
VIR_AUTOPTR(virDomain) dom = NULL;
and IIRC we ended up agreeing that it was a good idea overall.
> # define VIR_AUTOPTR(type) \
> __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
>
> The usage would not require our internal vir*Ptr types and would
> be easy to use with external types as well:
>
> VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
>
> VIR_AUTOPTR(virBitmap) map = NULL;
>
> VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
this is a reasonable usage, because we don't control the signature
of the libssh2_channel_free function.
This is why I suggested we might want to have two sets of
macros, one for types we defined ourselves and one for types
we bring in from external libraries.
> VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
With my example above
#define VIR_AUTOFREE_LIBSSH_CHANNEL VIR_AUTOFREE_FUNC(LIBSSH2_CHANNEL)
VIR_AUTOFREE_LIBSSH_CHANNEL LIBSSH_CHANNEL *chan = NULL;
This makes the declaration needlessly verbose, since you're
repeating the type name twice; Pavel's approach would avoid
that.
--
Andrea Bolognani / Red Hat / Virtualization
6:13 p.m.
On Mon, 18 Jun 2018 at 17:20, Andrea Bolognani <abologna(a)redhat.com> wrote:
On Mon, 2018-06-18 at 11:52 +0100, Daniel P. Berrangé wrote:
> On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> > # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > { \
> > (func)(_ptr); \
> > }
>
> For anything where we define the impl of (func) these two macros
> should never be used IMHO. We should just change the signature of
> the real functions so that accept a double pointer straight away,
> and thus not require the wrapper function. Yes, it this will
> require updating existing code, but such a design change is
> beneficial even without the cleanup macros, as it prevents the
> possbility of double frees. I'd suggest we just do this kind of
> change straightaway
Agreed that we want to fix our own free functions.
> > # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
>
> I don't see the point in passing "type" in here we could simplify
> this:
>
> #define VIR_AUTOFREE __attribute__((cleanup(virFree))
>
> VIR_AUTOFREE char *foo = NULL;
Passing the type was suggested earlier to make usage consistent
across all cases, eg.
VIR_AUTOFREE(char *) str = NULL;
VIR_AUTOPTR(virDomain) dom = NULL;
and IIRC we ended up agreeing that it was a good idea overall.
> > # define VIR_AUTOPTR(type) \
> > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
> >
> > The usage would not require our internal vir*Ptr types and would
> > be easy to use with external types as well:
> >
> > VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
> >
> > VIR_AUTOPTR(virBitmap) map = NULL;
> >
> > VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
>
> Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
> this is a reasonable usage, because we don't control the signature
> of the libssh2_channel_free function.
This is why I suggested we might want to have two sets of
macros, one for types we defined ourselves and one for types
we bring in from external libraries.
> > VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
>
> With my example above
>
> #define VIR_AUTOFREE_LIBSSH_CHANNEL VIR_AUTOFREE_FUNC(LIBSSH2_CHANNEL)
>
> VIR_AUTOFREE_LIBSSH_CHANNEL LIBSSH_CHANNEL *chan = NULL;
This makes the declaration needlessly verbose, since you're
repeating the type name twice; Pavel's approach would avoid
that.
So, have we reached a decision about the design?
Fixing the existing vir*Free functions to take double pointers seems good to me,
as many have mentioned earlier. It will solve multiple problems at once.
I have almost 7 weeks left of GSoC, and I think it is totally doable.
On the other hand, making cleanup macros use double pointers also seems
great! But, aren't we borrowing too much design from GLib?
Anyway, both approaches take care of the double free problem.
TL;DR
I prefer cleanup macros which define functions with double pointers
over changing
existing vir*Free functions. It just looks more natural.
I apologize in advance for this long mail! Could not decide what
patches to make,
so I included a glimpse of the code in this mail.
This is just to start this discussion again and help with visualizing.
Here goes some example code. Most of it is paraphrasing what is already
discussed previously.
/******************************************************************************
* Cleanup macros defining functions with double pointers:
******************************************************************************/
#define VIR_AUTOPTR_TYPE_NAME(type) type##AutoPtr
#define VIR_AUTOPTR_FUNC_NAME(type) \
VIR_AUTOPTR_TYPE_NAME(type)##Free
#define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
{ \
if (*_ptr) \
(func)(*_ptr); \
*_ptr = NULL; \
} \
#define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) \
VIR_AUTOPTR_TYPE_NAME(type)
///////// Using this for virBitmap:
VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree)
will generate:
typedef virBitmap *virBitmapAutoPtr;
static inline void virBitmapAutoPtrFree(virBitmap **_ptr)
{
if (*_ptr)
virBitmapFree(*_ptr);
*_ptr = NULL;
}
Then we use it like:
VIR_AUTOPTR(virBitmap) map = NULL;
which will become:
__attribute__((cleanup(virBitmapAutoPtrFree))) virBitmapAutoPtr map = NULL;
///////// Using this for char ** (some of my ideas added):
typedef char *virString;
VIR_DEFINE_AUTOPTR_FUNC(virString, virStringListFree)
will generate:
typedef virString *virStringAutoPtr;
static inline void virStringAutoPtrFree(virString **_ptr)
{
if (*_ptr)
virStringListFree(*_ptr);
*_ptr = NULL;
}
Then we use it like:
VIR_AUTOPTR(virString) lines = NULL;
which will become:
__attribute__((cleanup(virStringAutoPtrFree))) virStringAutoPtr lines = NULL;
///////// Using this for External types:
VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free)
will generate:
typedef LIBSSH2_CHANNEL *LIBSSH2_CHANNELAutoPtr;
static inline void LIBSSH2_CHANNELAutoPtrFree(LIBSSH2_CHANNEL **_ptr)
{
if (*_ptr)
libssh2_channel_free(*_ptr);
*_ptr = NULL;
}
Then we use it like:
VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
which will become:
__attribute__((cleanup(LIBSSH2_CHANNELAutoPtrFree)))
LIBSSH2_CHANNELAutoPtr channel = NULL;
/******************************************************************************
* Changing the existing vir*Free functions to accept double pointers:
* (not including the changes made in the above one, but reusing our
* original design)
******************************************************************************/
#define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
#define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
{ \
(func)(_ptr); \
} \
#define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
///////// Using this for virBitmap:
void
virBitmapFree(virBitmapPtr bitmap)
{
if (bitmap) {
VIR_FREE(bitmap->map);
VIR_FREE(bitmap);
}
}
will look like this:
void
virBitmapFree(virBitmapPtr *bitmap)
{
if (*bitmap) {
VIR_FREE((*bitmap)->map);
VIR_FREE(*bitmap);
}
*bitmap = NULL;
}
VIR_DEFINE_AUTOPTR_FUNC(virBitmapPtr, virBitmapFree)
will generate:
static inline void virAutoPtrvirBitmapPtr(virBitmapPtr *_ptr)
{
virBitmapFree(_ptr);
}
Then we use it like:
VIR_AUTOPTR(virBitmapPtr) map = NULL;
which will become:
__attribute__((cleanup(virBitmapPtrAutoPtrFree))) virBitmapPtr map = NULL;
But, then each (explicit) occurrence of this:
virBitmapFree(bitmap);
will have to be like this:
virBitmapFree(&bitmap);
///////// Using this for char **:
void virStringListFree(char **strings)
{
char **tmp = strings;
while (tmp && *tmp) {
VIR_FREE(*tmp);
tmp++;
}
VIR_FREE(strings);
}
will look like this:
void virStringListFree(char ***strings)
OR
void virStringListFree(virStringList *strings)
{
char ***tmp = strings;
OR
virStringList *tmp = strings;
while (*tmp && **tmp) {
VIR_FREE(**tmp);
*tmp++;
}
VIR_FREE(*strings);
*strings = NULL;
}
typedef char **virStringList;
VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree)
will generate:
static inline void virStringListAutoPtrFree(virStringList *_ptr)
{
virStringListFree(_ptr);
}
Then we use it like:
VIR_AUTOPTR(virStringList) lines = NULL;
which will become:
__attribute__((cleanup(virStringListAutoPtrFree))) virStringList lines = NULL;
But again, this:
virStringListFree(lines);
will have to be like this:
virStringListFree(&lines);
///////// For External types?:
We may need to make separate macros for External types. If so, we can
reuse the macros in the original design for them. Something like:
# define VIR_DEFINE_EXT_AUTOPTR_FUNC(type, func) \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
{ \
(func)(*_ptr); \
} \
# define VIR_EXT_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
Something like this might have to be done for all External types:
typedef LIBSSH2_CHANNEL *LIBSSH2_CHANNEL_PTR;
VIR_DEFINE_EXT_AUTOPTR_FUNC(LIBSSH2_CHANNEL_PTR, libssh2_channel_free);
will generate:
static inline void virAutoPtrLIBSSH2_CHANNEL_PTR(LIBSSH2_CHANNEL_PTR *_ptr)
{
libssh2_channel_free(*_ptr);
}
Then we use it like:
VIR_EXT_AUTOPTR(LIBSSH2_CHANNEL_PTR) channel = NULL;
which will become:
__attribute__((cleanup(virAutoPtrLIBSSH2_CHANNEL)))
LIBSSH2_CHANNEL_PTR channel = NULL;
Thanks,
Sukrit
3:36 a.m.
On Fri, Jun 22, 2018 at 04:43:35AM +0530, Sukrit Bhatnagar wrote:
On Mon, 18 Jun 2018 at 17:20, Andrea Bolognani
<abologna(a)redhat.com> wrote:
>
> On Mon, 2018-06-18 at 11:52 +0100, Daniel P. Berrangé wrote:
> > On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> > > # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > > static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > > { \
> > > (func)(_ptr); \
> > > }
> >
> > For anything where we define the impl of (func) these two macros
> > should never be used IMHO. We should just change the signature of
> > the real functions so that accept a double pointer straight away,
> > and thus not require the wrapper function. Yes, it this will
> > require updating existing code, but such a design change is
> > beneficial even without the cleanup macros, as it prevents the
> > possbility of double frees. I'd suggest we just do this kind of
> > change straightaway
>
> Agreed that we want to fix our own free functions.
>
> > > # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> >
> > I don't see the point in passing "type" in here we could
simplify
> > this:
> >
> > #define VIR_AUTOFREE __attribute__((cleanup(virFree))
> >
> > VIR_AUTOFREE char *foo = NULL;
>
> Passing the type was suggested earlier to make usage consistent
> across all cases, eg.
>
> VIR_AUTOFREE(char *) str = NULL;
> VIR_AUTOPTR(virDomain) dom = NULL;
>
> and IIRC we ended up agreeing that it was a good idea overall.
>
> > > # define VIR_AUTOPTR(type) \
> > > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
> > >
> > > The usage would not require our internal vir*Ptr types and would
> > > be easy to use with external types as well:
> > >
> > > VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
> > >
> > > VIR_AUTOPTR(virBitmap) map = NULL;
> > >
> > > VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
> >
> > Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
> > this is a reasonable usage, because we don't control the signature
> > of the libssh2_channel_free function.
>
> This is why I suggested we might want to have two sets of
> macros, one for types we defined ourselves and one for types
> we bring in from external libraries.
>
> > > VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
> >
> > With my example above
> >
> > #define VIR_AUTOFREE_LIBSSH_CHANNEL VIR_AUTOFREE_FUNC(LIBSSH2_CHANNEL)
> >
> > VIR_AUTOFREE_LIBSSH_CHANNEL LIBSSH_CHANNEL *chan = NULL;
>
> This makes the declaration needlessly verbose, since you're
> repeating the type name twice; Pavel's approach would avoid
> that.
So, have we reached a decision about the design?
Fixing the existing vir*Free functions to take double pointers seems good to me,
as many have mentioned earlier. It will solve multiple problems at once.
I have almost 7 weeks left of GSoC, and I think it is totally doable.
On the other hand, making cleanup macros use double pointers also seems
great! But, aren't we borrowing too much design from GLib?
Anyway, both approaches take care of the double free problem.
TL;DR
I prefer cleanup macros which define functions with double pointers
over changing
existing vir*Free functions. It just looks more natural.
It does, but it doesn't solve all the problems, mainly those Dan has in mind.
With attribute cleanup, it's applied on variables that go out of scope, but
that's not what you want if you want to keep the memory for while, imagine the
following:
A->B
where B:
<return type> B (<ptrptr> dst, ...)
{
VIR_AUTOPTR(tmp);
VIR_ALLOC(tmp)
...
*dst = tmp;
tmp = NULL;
return;
}
when A is finished with dst, it will need to free it manually --> this could
lead to a double free if A doesn't clear it afterwards, either in A or some
within some other consumer of dst.
So in the long term, we'll probably want to change the signatures, but as I
said, I see the proposed macros as an improvement worth being a stepping stone
towards changing the signatures of free functions. The ultimate goal is still
to gradually rewrite libvirt into some other memory-safe language.
Erik
3:22 a.m.
On Mon, Jun 18, 2018 at 11:52:04AM +0100, Daniel P. Berrangé wrote:
On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> Hi,
>
> It took me longer to sit down and write this mail but here it goes.
>
> There was a lot of ideas about the macro design and it's easy to
> get lost in all the designs.
>
> So we agreed on this form:
>
>
> # define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> # define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
>
> # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
> { \
> (func)(*_ptr); \
> }
>
> # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> { \
> (func)(_ptr); \
> }
For anything where we define the impl of (func) these two macros
should never be used IMHO. We should just change the signature of
the real functions so that accept a double pointer straight away,
and thus not require the wrapper function. Yes, it this will
require updating existing code, but such a design change is
beneficial even without the cleanup macros, as it prevents the
possbility of double frees. I'd suggest we just do this kind of
change straightaway
From security POV, I agree that the free functions should accept a
double
pointer. However, in terms of GSoC and the time schedule, I think that
having
the macros above is a nice stepping stone towards to long term goal to convert
our free functions to accept double pointers, besides, as you pointed out
below, the macros (at least the AUTOPTR_FUNC) makes sense with external types
and I like that we'd have a almost universal approach here. Even with the
macros, we don't lose anything (because they're straightforward and that's
important), quite the opposite, it's progress compared to the current status
quo.
> # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
I don't see the point in passing "type" in here we could simplify
this:
#define VIR_AUTOFREE __attribute__((cleanup(virFree))
VIR_AUTOFREE char *foo = NULL;
and at the same time fix the char ** problems
#define VIR_AUTOFREE_FUNC(func) __attribute__((cleanup(func))
VIR_AUTOFREE_FUNC(virStringListFree) char *foo = NULL;
or if we want to simplify it
#define VIR_AUTOFREE_STRINGLIST VIR_AUTOFREE_FUNC(virStringListFree)
VIR_AUTOFREE_STRINGLIST char **strs = NULL;
Andrea already pointed it out in his reply, but I don't like this
"simplification" either, the syntax is kinda obfuscating.
This also works for external libraries
> # define VIR_AUTOPTR(type) \
> __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
>
> # define VIR_AUTOCLEAR(type) \
> __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
>
>
> but it turned out the it's not sufficient for several reasons:
>
> - there is no simple way ho to free list of strings (char **)
> - this will not work for external types with their own free function
>
>
> In order to solve these two issues there were some ideas.
>
>
> 1. How to handle list of strings
>
> We have virStringListFree() function in order to free list of strings,
> the question is how to use it with these macros:
>
> - Create a new typedef for list of strings and use VIR_AUTOPTR:
>
> typedef char **virStringList;
>
> VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
>
> VIR_AUTOPTR(virStringList) list = NULL;
I don't think we should be creating artifical new typedefs just to
deal with the flawed design of our own autofree macros.
> - Overload VIR_AUTOFREE macro by making it variadic
>
> # define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
> # define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
>
> # define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
> # define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__, _VIR_AUTOFREE_1,
_VIR_AUTOFREE_0)(__VA_ARGS__)
This is uneccessarily black magic - just have VIR_AUTOFREE and
VIR_AUTOFREE_FUNC defined separately.
Yeah, looking back at when I came up with the idea, it is indeed unnecessarily
hacky. Having another macro for this purpose is an option, although Pavel had
some concerns that if we do that, people might misuse it every time they don't
know what the proper usage or approach is in that the type name and the
corresponding free function name should match. To me, that sounds like a
reviewer's job to spot. On the other hand, I think that the virStringList type
would truly be an exception here as it's unlikely that we'd be creating an
integer pointer-based list with everything else already being a compound type.
So, I'm still ambivalent here and I'd be fine with both having either an extra
macro or an extra typedef.
> void virStringListPtrFree(char ***stringsp)
> {
> virStringListFree(*stringsp);
> }
As above, we should just fixed virStringListFree to have the better
signature straight away.
> VIR_AUTOFREE(char **, virStringListPtrFree) list = NULL;
>
>
> 2. External types with free function
>
> Libvirt uses a lot of external libraries where we use the types and
> relevant free functions. The issue with original design is that it was
> counting on the fact that we would have vir*Ptr typedefs, but that's not
> the case for external types.
>
> - We can modify the design to be closer to GLib design which would
> work even with external types and would be safe in case external
> free functions will not behave correctly. These are to
> modification to the original design:
>
> # define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
>
> # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> typedef type *VIR_AUTOPTR_TYPE_NAME(type);
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> { \
> if (*_ptr) { \
> (func)(*_ptr); \
> *_ptr = NULL; \
> } \
> }
>
> # define VIR_AUTOPTR(type) \
> __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
>
> The usage would not require our internal vir*Ptr types and would
> be easy to use with external types as well:
>
> VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
>
> VIR_AUTOPTR(virBitmap) map = NULL;
>
> VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
this is a reasonable usage, because we don't control the signature
of the libssh2_channel_free function.
> VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
With my example above
#define VIR_AUTOFREE_LIBSSH_CHANNEL VIR_AUTOFREE_FUNC(LIBSSH2_CHANNEL)
VIR_AUTOFREE_LIBSSH_CHANNEL LIBSSH_CHANNEL *chan = NULL;
As I noted above, ^this specific example is rather unreadable and again we
should stick to the consistent convention MACRO(args) we agreed upon earlier.
In conclusion, this discussion has been going for a while with no apparent
progress, there have been a few proposals (it doesn't look there are going to
be any other ones) so now we should decide which direction we're going to head.
Overall, I like the approach in Pavel's summary as to me it's the cleanest
looking solutions of all, with the exception of the virStringList vs
VIR_AUTOFREE_FUNC issue, where I prefer the latter since it can happily coexist
with the rest, but having the former is not a show stopper to me either.
Thanks,
Erik
1:31 a.m.
On Mon, Jun 25, 2018 at 10:22:14AM +0200, Erik Skultety wrote:
On Mon, Jun 18, 2018 at 11:52:04AM +0100, Daniel P. Berrangé wrote:
> On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> > Hi,
> >
> > It took me longer to sit down and write this mail but here it goes.
> >
> > There was a lot of ideas about the macro design and it's easy to
> > get lost in all the designs.
> >
> > So we agreed on this form:
> >
> >
> > # define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> > # define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
> >
> > # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
> > { \
> > (func)(*_ptr); \
> > }
> >
> > # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > { \
> > (func)(_ptr); \
> > }
>
> For anything where we define the impl of (func) these two macros
> should never be used IMHO. We should just change the signature of
> the real functions so that accept a double pointer straight away,
> and thus not require the wrapper function. Yes, it this will
> require updating existing code, but such a design change is
> beneficial even without the cleanup macros, as it prevents the
> possbility of double frees. I'd suggest we just do this kind of
> change straightaway
>From security POV, I agree that the free functions should accept a double
pointer. However, in terms of GSoC and the time schedule, I think that having
the macros above is a nice stepping stone towards to long term goal to convert
our free functions to accept double pointers, besides, as you pointed out
below, the macros (at least the AUTOPTR_FUNC) makes sense with external types
and I like that we'd have a almost universal approach here. Even with the
macros, we don't lose anything (because they're straightforward and that's
important), quite the opposite, it's progress compared to the current status
quo.
>
> > # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
>
> I don't see the point in passing "type" in here we could simplify
> this:
>
> #define VIR_AUTOFREE __attribute__((cleanup(virFree))
>
> VIR_AUTOFREE char *foo = NULL;
>
> and at the same time fix the char ** problems
>
> #define VIR_AUTOFREE_FUNC(func) __attribute__((cleanup(func))
>
> VIR_AUTOFREE_FUNC(virStringListFree) char *foo = NULL;
>
> or if we want to simplify it
>
> #define VIR_AUTOFREE_STRINGLIST VIR_AUTOFREE_FUNC(virStringListFree)
>
> VIR_AUTOFREE_STRINGLIST char **strs = NULL;
Andrea already pointed it out in his reply, but I don't like this
"simplification" either, the syntax is kinda obfuscating.
>
>
> This also works for external libraries
>
>
>
> > # define VIR_AUTOPTR(type) \
> > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
> >
> > # define VIR_AUTOCLEAR(type) \
> > __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
> >
> >
> > but it turned out the it's not sufficient for several reasons:
> >
> > - there is no simple way ho to free list of strings (char **)
> > - this will not work for external types with their own free function
> >
> >
> > In order to solve these two issues there were some ideas.
> >
> >
> > 1. How to handle list of strings
> >
> > We have virStringListFree() function in order to free list of strings,
> > the question is how to use it with these macros:
> >
> > - Create a new typedef for list of strings and use VIR_AUTOPTR:
> >
> > typedef char **virStringList;
> >
> > VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
> >
> > VIR_AUTOPTR(virStringList) list = NULL;
>
> I don't think we should be creating artifical new typedefs just to
> deal with the flawed design of our own autofree macros.
>
> > - Overload VIR_AUTOFREE macro by making it variadic
> >
> > # define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
> > # define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
> >
> > # define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
> > # define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__,
_VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)
>
> This is uneccessarily black magic - just have VIR_AUTOFREE and
> VIR_AUTOFREE_FUNC defined separately.
Yeah, looking back at when I came up with the idea, it is indeed unnecessarily
hacky. Having another macro for this purpose is an option, although Pavel had
some concerns that if we do that, people might misuse it every time they don't
know what the proper usage or approach is in that the type name and the
corresponding free function name should match. To me, that sounds like a
reviewer's job to spot. On the other hand, I think that the virStringList type
would truly be an exception here as it's unlikely that we'd be creating an
integer pointer-based list with everything else already being a compound type.
So, I'm still ambivalent here and I'd be fine with both having either an extra
macro or an extra typedef.
>
> > void virStringListPtrFree(char ***stringsp)
> > {
> > virStringListFree(*stringsp);
> > }
>
> As above, we should just fixed virStringListFree to have the better
> signature straight away.
>
> > VIR_AUTOFREE(char **, virStringListPtrFree) list = NULL;
> >
> >
> > 2. External types with free function
> >
> > Libvirt uses a lot of external libraries where we use the types and
> > relevant free functions. The issue with original design is that it was
> > counting on the fact that we would have vir*Ptr typedefs, but that's not
> > the case for external types.
> >
> > - We can modify the design to be closer to GLib design which would
> > work even with external types and would be safe in case external
> > free functions will not behave correctly. These are to
> > modification to the original design:
> >
> > # define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
> >
> > # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > typedef type *VIR_AUTOPTR_TYPE_NAME(type);
> > static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> > { \
> > if (*_ptr) { \
> > (func)(*_ptr); \
> > *_ptr = NULL; \
> > } \
> > }
> >
> > # define VIR_AUTOPTR(type) \
> > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type))))
VIR_AUTOPTR_TYPE_NAME(type)
> >
> > The usage would not require our internal vir*Ptr types and would
> > be easy to use with external types as well:
> >
> > VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
> >
> > VIR_AUTOPTR(virBitmap) map = NULL;
> >
> > VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
>
> Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
> this is a reasonable usage, because we don't control the signature
> of the libssh2_channel_free function.
>
>
> > VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
>
> With my example above
>
> #define VIR_AUTOFREE_LIBSSH_CHANNEL VIR_AUTOFREE_FUNC(LIBSSH2_CHANNEL)
>
> VIR_AUTOFREE_LIBSSH_CHANNEL LIBSSH_CHANNEL *chan = NULL;
As I noted above, ^this specific example is rather unreadable and again we
should stick to the consistent convention MACRO(args) we agreed upon earlier.
In conclusion, this discussion has been going for a while with no apparent
progress, there have been a few proposals (it doesn't look there are going to
be any other ones) so now we should decide which direction we're going to head.
Overall, I like the approach in Pavel's summary as to me it's the cleanest
looking solutions of all, with the exception of the virStringList vs
VIR_AUTOFREE_FUNC issue, where I prefer the latter since it can happily coexist
with the rest, but having the former is not a show stopper to me either.
I have nothing else to add so ACK to all of that.
Pavel
2341
days inactive
2358
days old
20 comments
6 participants
participants (6)
-
Andrea Bolognani -
Daniel P. Berrangé -
Erik Skultety -
Martin Kletzander -
Pavel Hrdina -
Sukrit Bhatnagar