[libvirt] [GSoC] Code design for scalar and external types
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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.
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
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 ;)
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
On Mon, 11 Jun 2018 at 16:35, Pavel Hrdina <phrdina@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?
On Mon, Jun 11, 2018 at 11:59:06PM +0530, Sukrit Bhatnagar wrote:
On Mon, 11 Jun 2018 at 16:35, Pavel Hrdina <phrdina@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
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
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
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
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.
[1] https://www.redhat.com/archives/libvir-list/2018-June/msg00033.html
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 :|
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
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
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 :|
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
On Mon, 18 Jun 2018 at 17:20, Andrea Bolognani <abologna@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
On Fri, Jun 22, 2018 at 04:43:35AM +0530, Sukrit Bhatnagar wrote:
On Mon, 18 Jun 2018 at 17:20, Andrea Bolognani <abologna@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
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
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
participants (6)
-
Andrea Bolognani -
Daniel P. Berrangé -
Erik Skultety -
Martin Kletzander -
Pavel Hrdina -
Sukrit Bhatnagar