[libvirt] [GSoC] Design ideas for implementing cleanup attribute
Hi, I am interested in implementing the GCC cleanup attribute for automatic resource freeing as part of GSoC'18. I have shared a proposal for the same. This mail is to discuss the code design for implementing it. Here are some of my ideas: This attribute requires a cleanup function that is called automatically when the corresponding variable goes out of scope. There are some functions whose logic can be reused: - Functions such as virCommandFree, virConfFreeList and virCgroupFree can be directly used as cleanup functions. They have parameter and return type valid for a cleanup function. - Functions such as virFileClose and virFileFclose need some additional consideration as they return a value. I think we can set some global variable in a separate source file (just like errno variable from errno.h). Then the value to be returned can be accessed globally. - Functions such as virDomainEventGraphicsDispose need an entirely new design. They are used as callbacks in object classes and passed as an argument in virClassNew. This would require making changes to virObjectUnref's code too. *This is the part I am not sure how to implement cleanup logic for.* Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro like autoclean (ideas for macro name welcome!) can be used instead. As Martin pointed out in my proposal, for some types, this can be done right after typedef declarations, so that the type itself contains this attribute. Basically, at most places where VIR_FREE is used to release memory explicitly, the corresponding variable can use the attribute. The existing virFree function also can be reused as it takes void pointer as an argument and returns nothing. One of the exceptions to this will be those variables which are struct members. The cleanup of member has to be done when the enclosing struct variable is cleaned. I can create new files vircleanup.{c,h} for defining cleanup functions for types which do not have an existing cleanup/free function. This can be done separately for each driver supported. For example, cleanups pertaining to lxc driver will be in src/lxc/lxc_cleanup.c. Your suggestions are welcome. Thanks, Sukrit Bhatnagar
On Sun, Mar 25, 2018 at 01:55:07AM +0530, Sukrit Bhatnagar wrote:
Hi,
I am interested in implementing the GCC cleanup attribute for automatic resource freeing as part of GSoC'18. I have shared a proposal for the same.
This mail is to discuss the code design for implementing it.
Here are some of my ideas:
This attribute requires a cleanup function that is called automatically when the corresponding variable goes out of scope. There are some functions whose logic can be reused:
- Functions such as virCommandFree, virConfFreeList and virCgroupFree can be directly used as cleanup functions. They have parameter and return type valid for a cleanup function.
- Functions such as virFileClose and virFileFclose need some additional consideration as they return a value. I think we can set some global variable in a separate source file (just like errno variable from errno.h). Then the value to be returned can be accessed globally.
Note we call VIR_FORCE_CLOSE and VIR_FORCE_FCLOSE in cleanup scenarios to explicitly ignore the return value. We only care about checking return value for VIR_CLOSE and VIR_FCLOSE in a tiny set of cases.
- Functions such as virDomainEventGraphicsDispose need an entirely new design. They are used as callbacks in object classes and passed as an argument in virClassNew. This would require making changes to virObjectUnref's code too. *This is the part I am not sure how to implement cleanup logic for.*
There shouldn't be any need to touch the DIspose funtions - that's internal helper code. To "free" an object variable, you just want to end up invoking the regular virObjectUnref function as the cleanup func.
Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro like autoclean (ideas for macro name welcome!) can be used instead. As Martin pointed out in my proposal, for some types, this can be done right after typedef declarations, so that the type itself contains this attribute.
Just suffix it with "Auto", eg virDomainAutoPtr, vs virDomainPtr.
I can create new files vircleanup.{c,h} for defining cleanup functions for types which do not have an existing cleanup/free function. This can be done separately for each driver supported. For example, cleanups pertaining to lxc driver will be in src/lxc/lxc_cleanup.c.
I'm not rally seeing a compelling need for this - just add it in appropriate existing files. 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 Sun, Mar 25, 2018 at 01:55:07AM +0530, Sukrit Bhatnagar wrote:
Hi,
I am interested in implementing the GCC cleanup attribute for automatic resource freeing as part of GSoC'18. I have shared a proposal for the same.
This mail is to discuss the code design for implementing it.
Here are some of my ideas:
This attribute requires a cleanup function that is called automatically when the corresponding variable goes out of scope. There are some functions whose logic can be reused:
- Functions such as virCommandFree, virConfFreeList and virCgroupFree can be directly used as cleanup functions. They have parameter and return type valid for a cleanup function.
- Functions such as virFileClose and virFileFclose need some additional consideration as they return a value. I think we can set some global variable in a separate source file (just like errno variable from errno.h). Then the value to be returned can be accessed globally.
- Functions such as virDomainEventGraphicsDispose need an entirely new design. They are used as callbacks in object classes and passed as an argument in virClassNew. This would require making changes to virObjectUnref's code too. *This is the part I am not sure how to implement cleanup logic for.*
Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro like autoclean (ideas for macro name welcome!) can be used instead. As Martin pointed out in my proposal, for some types, this can be done right after typedef declarations, so that the type itself contains this attribute.
Basically, at most places where VIR_FREE is used to release memory explicitly, the corresponding variable can use the attribute. The existing virFree function also can be reused as it takes void pointer as an argument and returns nothing. One of the exceptions to this will be those variables which are struct members. The cleanup of member has to be done when the enclosing struct variable is cleaned.
I can create new files vircleanup.{c,h} for defining cleanup functions for types which do not have an existing cleanup/free function. This can be done separately for each driver supported. For example, cleanups pertaining to lxc driver will be in src/lxc/lxc_cleanup.c.
Hi, I would like to apologize that it took me that long to reply. We've already discussed this a little bit off-list so I would like to summarize my idea about the design of this effort. I liked the way how GLib is solving the issue so we can simply use the same approach since it looks reasonable. There would be three different macros that would be used to annotate variable with attribute cleanup: VIR_AUTOFREE char *str = NULL; - this would call virFree on that variable VIR_AUTOPTR(virDomain) domain = NULL; - this would call registered free function on that variable - to register the free function you would use: VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree); VIR_AUTOCLEAR(virDomain) domain = { 0 }; - this would call registered clear function to free the content of that structure - to register that clear function you would use: VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear); In GLib there are three "define" macros and all of them creates a wrapper function to make sure that the Free/Clear function is called only if necessary. This is a safe-guard because it's a public API. We don't have to have this safe-guard in place since almost all of our Free functions already check whether the pointer is null or not. For reference how it works in GLib you can look here [1]. Pavel [1] <https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html>
On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote: [...]
I liked the way how GLib is solving the issue so we can simply use the same approach since it looks reasonable.
There would be three different macros that would be used to annotate variable with attribute cleanup:
VIR_AUTOFREE char *str = NULL;
For consistency I'd prefer if the argument is in parentheses similarly to the ones below.
- this would call virFree on that variable
VIR_AUTOPTR(virDomain) domain = NULL;
- this would call registered free function on that variable - to register the free function you would use:
VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
Did you mean virDomainPtr? How is the type matched? Does it have to be a typedef'd type to use this?
On Wed, May 23, 2018 at 06:23:01PM +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
[...]
I liked the way how GLib is solving the issue so we can simply use the same approach since it looks reasonable.
There would be three different macros that would be used to annotate variable with attribute cleanup:
VIR_AUTOFREE char *str = NULL;
For consistency I'd prefer if the argument is in parentheses similarly to the ones below.
Yes, for consistency it can take the type as an argument but it's not necessary since the type is not used by that macro.
- this would call virFree on that variable
VIR_AUTOPTR(virDomain) domain = NULL;
- this would call registered free function on that variable - to register the free function you would use:
VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
Did you mean virDomainPtr?
Right, it can be virDomainPtr since we already have these typedefs, GLib implementation actually creates the same typedef inside the G_DEFINE_AUTOPTR_CLEANUP_FUNC macro, we don't have to do it. It also creates a wrapper function with a name based on the passed type and that wrapper function is then used by g_autoptr() again based on the passed type. Pavel
On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote: [...]
VIR_AUTOFREE char *str = NULL;
For consistency I'd prefer if the argument is in parentheses similarly to the ones below.
Seconded. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, May 25, 2018 at 09:01:03AM +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote: [...]
VIR_AUTOFREE char *str = NULL;
For consistency I'd prefer if the argument is in parentheses similarly to the ones below.
Seconded.
Well that would mean having this macro: #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type * and the usage would be: VIR_AUTOFREE(char) string = NULL; Yes, for consistency it make sense but sometimes exception makes it look better and IMHO this is the case so I would prefer #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) and VIR_AUTOFREE char *string = NULL; The only sensible usage would be to have only VIR_AUTOPTR as you've mentioned in other mail, but that would require a lot of typedefs. Pavel
On Fri, 2018-05-25 at 10:08 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:01:03AM +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
[...]
VIR_AUTOFREE char *str = NULL;
For consistency I'd prefer if the argument is in parentheses similarly to the ones below.
Seconded.
Well that would mean having this macro:
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type *
and the usage would be:
VIR_AUTOFREE(char) string = NULL;
Yes, for consistency it make sense but sometimes exception makes it look better and IMHO this is the case so I would prefer
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree)))
and
VIR_AUTOFREE char *string = NULL;
I'm probably missing something, but couldn't you just have #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type which you would then use as VIR_AUTOFREE(char *) string = NULL; instead? -- Andrea Bolognani / Red Hat / Virtualization
On Fri, May 25, 2018 at 10:32:04AM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:08 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:01:03AM +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
[...]
VIR_AUTOFREE char *str = NULL;
For consistency I'd prefer if the argument is in parentheses similarly to the ones below.
Seconded.
Well that would mean having this macro:
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type *
and the usage would be:
VIR_AUTOFREE(char) string = NULL;
Yes, for consistency it make sense but sometimes exception makes it look better and IMHO this is the case so I would prefer
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree)))
and
VIR_AUTOFREE char *string = NULL;
I'm probably missing something, but couldn't you just have
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
which you would then use as
VIR_AUTOFREE(char *) string = NULL;
instead?
Yes you can have that as well, but it doesn't look ugly to you? :) If majority will agree on that I don't care, just my opinion. Pavel
On Fri, 2018-05-25 at 10:46 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 10:32:04AM +0200, Andrea Bolognani wrote:
I'm probably missing something, but couldn't you just have
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
which you would then use as
VIR_AUTOFREE(char *) string = NULL;
instead?
Yes you can have that as well, but it doesn't look ugly to you? :)
Quite the opposite - not only it's consistent with the other macros, but it also cleanly separates the type from the variable name, which I consider a plus. Personally, even though char *string; and friends are the accepted way to declare pointer variables, I've always been slightly annoyed by the fact that type name and variable name end up being partially mixed together. Don't get me wrong, something like char* string; would still look wrong to me, because I'm just so used to the other way! But in the context of that macro I think we really get the best of both worlds :)
If majority will agree on that I don't care, just my opinion.
Yeah, same here. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, May 25, 2018 at 11:03:01AM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:46 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 10:32:04AM +0200, Andrea Bolognani wrote:
I'm probably missing something, but couldn't you just have
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
which you would then use as
VIR_AUTOFREE(char *) string = NULL;
instead?
Yes you can have that as well, but it doesn't look ugly to you? :)
Quite the opposite - not only it's consistent with the other macros, but it also cleanly separates the type from the variable name, which I consider a plus.
I also like the alternative approach Andrea mentioned, especially since you can reuse it for structures using plain scalar types, e.g. a structure like typedef _virSomething virSomething; typedef virSomething * virSomethingPtr; struct _virSomething { int a; long b; bool c; }; ..where you don't really need to use VIR_AUTOPTR's complex clean functions since a simple virFree works just fine and it lets you to stay consistent with the usage of XPtr types which has also been mentioned already, just my 2c. Erik
On Fri, 2018-05-25 at 11:17 +0200, Erik Skultety wrote:
I also like the alternative approach Andrea mentioned, especially since you can reuse it for structures using plain scalar types, e.g. a structure like
typedef _virSomething virSomething; typedef virSomething * virSomethingPtr; struct _virSomething { int a; long b; bool c; };
..where you don't really need to use VIR_AUTOPTR's complex clean functions since a simple virFree works just fine and it lets you to stay consistent with the usage of XPtr types which has also been mentioned already, just my 2c.
I have to disagree here: I think every virSomething should have a corresponding virSomethingFree() function, even when it ends up doing nothing but calling VIR_FREE() or virObjectUnref(). Even when automatic cleaning will be in place, there will still be cases in which you have to allocate an object and return it for the caller to free, and not having to look up what specific free function needs to be used because you can rely on the existence of a consistently named one is a big plus in my book. We're pretty bad at this already, let's not make it worse :) -- Andrea Bolognani / Red Hat / Virtualization
On 23 May 2018 at 21:35, Pavel Hrdina <phrdina@redhat.com> wrote:
On Sun, Mar 25, 2018 at 01:55:07AM +0530, Sukrit Bhatnagar wrote:
Hi,
I am interested in implementing the GCC cleanup attribute for automatic resource freeing as part of GSoC'18. I have shared a proposal for the same.
This mail is to discuss the code design for implementing it.
Here are some of my ideas:
This attribute requires a cleanup function that is called automatically when the corresponding variable goes out of scope. There are some functions whose logic can be reused:
- Functions such as virCommandFree, virConfFreeList and virCgroupFree can be directly used as cleanup functions. They have parameter and return type valid for a cleanup function.
- Functions such as virFileClose and virFileFclose need some additional consideration as they return a value. I think we can set some global variable in a separate source file (just like errno variable from errno.h). Then the value to be returned can be accessed globally.
- Functions such as virDomainEventGraphicsDispose need an entirely new design. They are used as callbacks in object classes and passed as an argument in virClassNew. This would require making changes to virObjectUnref's code too. *This is the part I am not sure how to implement cleanup logic for.*
Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro like autoclean (ideas for macro name welcome!) can be used instead. As Martin pointed out in my proposal, for some types, this can be done right after typedef declarations, so that the type itself contains this attribute.
Basically, at most places where VIR_FREE is used to release memory explicitly, the corresponding variable can use the attribute. The existing virFree function also can be reused as it takes void pointer as an argument and returns nothing. One of the exceptions to this will be those variables which are struct members. The cleanup of member has to be done when the enclosing struct variable is cleaned.
I can create new files vircleanup.{c,h} for defining cleanup functions for types which do not have an existing cleanup/free function. This can be done separately for each driver supported. For example, cleanups pertaining to lxc driver will be in src/lxc/lxc_cleanup.c.
Hi,
I would like to apologize that it took me that long to reply.
We've already discussed this a little bit off-list so I would like to summarize my idea about the design of this effort.
I liked the way how GLib is solving the issue so we can simply use the same approach since it looks reasonable.
There would be three different macros that would be used to annotate variable with attribute cleanup:
VIR_AUTOFREE char *str = NULL;
- this would call virFree on that variable
This seems fine for basic native datatypes. But for the complex types, why can't we do something like the following, as was discussed previously? * Define simple macros for the attribute per struct type: #define VIRCOMMAND_AUTOFREE __attribute__((__cleanup__(freefunction)) #define VIRCOMMAND_AUTOCLEAN __attribute__((__cleanup__(cleanfunction)) * Create new datatypes which include this attribute: #define virCommandAutoFreePtr VIRCOMMAND_AUTOFREE virCommandPtr #define virCommandAutoCleanPtr VIRCOMMAND_AUTOCLEAN virCommandPtr * Then simply declare variables as: virCommandAutoFreePtr cmd = NULL; We just have to make sure that all Ptr variables are initialized, atleast to NULL. Also, we can create and use macros to initialize the variables: #define CREATE_CMD_PTR(var, val) \ VIRCOMMAND_AUTOFREE virCommandPtr var = (val); CREATE_CMD_PTR(cmd, virCommandNewArgs(args)) This is equivalent to: VIRCOMMAND_AUTOFREE virCommandPtr cmd = virCommandNewArgs(args); Many existing Free functions such as virDomainFree, return a value which is not allowed for a function that is to be used with cleanup attribute. Do we ignore all such return values?
VIR_AUTOPTR(virDomain) domain = NULL;
- this would call registered free function on that variable - to register the free function you would use:
VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
VIR_AUTOCLEAR(virDomain) domain = { 0 };
- this would call registered clear function to free the content of that structure - to register that clear function you would use:
VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
In GLib there are three "define" macros and all of them creates a wrapper function to make sure that the Free/Clear function is called only if necessary. This is a safe-guard because it's a public API. We don't have to have this safe-guard in place since almost all of our Free functions already check whether the pointer is null or not.
For reference how it works in GLib you can look here [1].
Pavel
[1] <https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html>
On Thu, May 24, 2018 at 11:16:40PM +0530, Sukrit Bhatnagar wrote:
On 23 May 2018 at 21:35, Pavel Hrdina <phrdina@redhat.com> wrote:
On Sun, Mar 25, 2018 at 01:55:07AM +0530, Sukrit Bhatnagar wrote:
Hi,
I am interested in implementing the GCC cleanup attribute for automatic resource freeing as part of GSoC'18. I have shared a proposal for the same.
This mail is to discuss the code design for implementing it.
Here are some of my ideas:
This attribute requires a cleanup function that is called automatically when the corresponding variable goes out of scope. There are some functions whose logic can be reused:
- Functions such as virCommandFree, virConfFreeList and virCgroupFree can be directly used as cleanup functions. They have parameter and return type valid for a cleanup function.
- Functions such as virFileClose and virFileFclose need some additional consideration as they return a value. I think we can set some global variable in a separate source file (just like errno variable from errno.h). Then the value to be returned can be accessed globally.
- Functions such as virDomainEventGraphicsDispose need an entirely new design. They are used as callbacks in object classes and passed as an argument in virClassNew. This would require making changes to virObjectUnref's code too. *This is the part I am not sure how to implement cleanup logic for.*
Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro like autoclean (ideas for macro name welcome!) can be used instead. As Martin pointed out in my proposal, for some types, this can be done right after typedef declarations, so that the type itself contains this attribute.
Basically, at most places where VIR_FREE is used to release memory explicitly, the corresponding variable can use the attribute. The existing virFree function also can be reused as it takes void pointer as an argument and returns nothing. One of the exceptions to this will be those variables which are struct members. The cleanup of member has to be done when the enclosing struct variable is cleaned.
I can create new files vircleanup.{c,h} for defining cleanup functions for types which do not have an existing cleanup/free function. This can be done separately for each driver supported. For example, cleanups pertaining to lxc driver will be in src/lxc/lxc_cleanup.c.
Hi,
I would like to apologize that it took me that long to reply.
We've already discussed this a little bit off-list so I would like to summarize my idea about the design of this effort.
I liked the way how GLib is solving the issue so we can simply use the same approach since it looks reasonable.
There would be three different macros that would be used to annotate variable with attribute cleanup:
VIR_AUTOFREE char *str = NULL;
- this would call virFree on that variable
This seems fine for basic native datatypes.
But for the complex types, why can't we do something like the following, as was discussed previously?
* Define simple macros for the attribute per struct type:
#define VIRCOMMAND_AUTOFREE __attribute__((__cleanup__(freefunction)) #define VIRCOMMAND_AUTOCLEAN __attribute__((__cleanup__(cleanfunction))
* Create new datatypes which include this attribute:
#define virCommandAutoFreePtr VIRCOMMAND_AUTOFREE virCommandPtr #define virCommandAutoCleanPtr VIRCOMMAND_AUTOCLEAN virCommandPtr
* Then simply declare variables as:
virCommandAutoFreePtr cmd = NULL;
We just have to make sure that all Ptr variables are initialized, atleast to NULL.
Also, we can create and use macros to initialize the variables:
#define CREATE_CMD_PTR(var, val) \ VIRCOMMAND_AUTOFREE virCommandPtr var = (val); CREATE_CMD_PTR(cmd, virCommandNewArgs(args))
This is equivalent to: VIRCOMMAND_AUTOFREE virCommandPtr cmd = virCommandNewArgs(args);
Yes, this solution would work but it looks way too complicated. It would require to create new macro for every structure that we have in libvirt which seems redundant if we can have few universal macros that can be used for everything. Each macro should be simple and should do one thing and should not hide too much logic. The design that I was proposing would be used like this for the virCommandPtr: src/util/viralloc.h: #define VIR_AUTOFREE __attribute__((cleanup(virFree))) #define VIR_AUTOPTR(type) ... #define VIR_AUTOCLEAR(type) ... #define VIR_DEFINE_AUTOPTR_FUNC(type, freeFunc) ... #define VIR_DEFINE_AUTOCLEAR_FUNC(type, clearFunc) ... src/util/vircommand.h: VIR_DEFINE_AUTOPTR_FUNC(virCommandPtr, virCommandFree); src/qemu/qemu_driver.c: (the virCommandPtr is used there) ... VIR_AUTOPTR(virCommandPtr) cmd = NULL; ... Now the fist design would require to introduce a lot of new macros to achieve the same result, let's summarize it for comparison: src/util/vircommand.h: #define VIRCOMMAND_AUTOFREE __attribute__((__cleanup__(virCommandFree)) #define virCommandAutoFreePtr VIRCOMMAND_AUTOFREE virCommandPtr //optional #define CREATE_CMD_PTR(var, val) \ VIRCOMMAND_AUTOFREE virCommandPtr var = (val); src/qemu/qemu_driver.c: ... virCommandAutoFreePtr cmd = NULL; ... or ... CREATE_CMD_PTR(cmd, virCommandNewArgs(args)) ... As you can see, the original solution doesn't have any generic macro but it requires at least 2 new macros for every single structure whereas the new design requires only 1 call of existing macro for every structure. It's more flexible and all the logic is only in once place.
Many existing Free functions such as virDomainFree, return a value which is not allowed for a function that is to be used with cleanup attribute. Do we ignore all such return values?
virDomainFree is a public API and we don't have to use them in our internal code so this should not be an issue. There are some internal free functions that also return some value, for example virObjectUnref, but that is not an issue as well. Look at the documentation of GLib functions and also at the code to get the idea of how it works. Pavel
On Wed, 2018-05-23 at 18:05 +0200, Pavel Hrdina wrote:
I liked the way how GLib is solving the issue so we can simply use the same approach since it looks reasonable.
There would be three different macros that would be used to annotate variable with attribute cleanup:
VIR_AUTOFREE char *str = NULL;
- this would call virFree on that variable
VIR_AUTOPTR(virDomain) domain = NULL;
- this would call registered free function on that variable - to register the free function you would use:
VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
VIR_AUTOCLEAR(virDomain) domain = { 0 };
- this would call registered clear function to free the content of that structure - to register that clear function you would use:
VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
I assume you would get a compilation error when trying to eg. use VIR_AUTOCLEAR() with a type that doesn't have a clear function registered? As for VIR_AUTOFREE() and VIR_AUTOPTR(), I'd very much prefer if we could have a single macro, since from the high-level point of view they're both doing the same thing, that is, freeing memory that was allocated on the heap. However, I realize it might not be possible to register free functions for a native type without having to introduce something like typedef char * virString; thus causing massive churn. How does GLib deal with that? -- Andrea Bolognani / Red Hat / Virtualization
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:05 +0200, Pavel Hrdina wrote:
I liked the way how GLib is solving the issue so we can simply use the same approach since it looks reasonable.
There would be three different macros that would be used to annotate variable with attribute cleanup:
VIR_AUTOFREE char *str = NULL;
- this would call virFree on that variable
VIR_AUTOPTR(virDomain) domain = NULL;
- this would call registered free function on that variable - to register the free function you would use:
VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
VIR_AUTOCLEAR(virDomain) domain = { 0 };
- this would call registered clear function to free the content of that structure - to register that clear function you would use:
VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
I assume you would get a compilation error when trying to eg. use VIR_AUTOCLEAR() with a type that doesn't have a clear function registered?
Yes, because the macro would call non existing function.
As for VIR_AUTOFREE() and VIR_AUTOPTR(), I'd very much prefer if we could have a single macro, since from the high-level point of view they're both doing the same thing, that is, freeing memory that was allocated on the heap.
It would be nice but I don't think it's worth it.
However, I realize it might not be possible to register free functions for a native type without having to introduce something like
typedef char * virString;
thus causing massive churn. How does GLib deal with that?
If you would look into GLib documentation you would see that this design basically copies the one in GLib: GLib libvirt g_autofree VIR_AUTOFREE g_autoptr VIR_AUTOPTR g_auto VIR_AUTOCLEAR In GLib you are using them like this: g_autofree char *string = NULL; g_autoptr(virDomain) dom = NULL; g_auto(virDomain) dom = { 0 }; So yes it would require to introduce a lot of typedefs for basic types and that is not worth it. In libvirt we would have: VIR_AUTOFREE char *string = NULL; VIR_AUTOPTR(virDomainPtr) dom = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 }; If you notice the difference, in libvirt we can use virDomainPtr directly because we have these typedefs, in GLib macro G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef. Pavel
On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
However, I realize it might not be possible to register free functions for a native type without having to introduce something like
typedef char * virString;
thus causing massive churn. How does GLib deal with that?
If you would look into GLib documentation you would see that this design basically copies the one in GLib:
Sorry, I should have looked up the documentation and implementation before asking silly questions. Guess the morning coffee hadn't quite kicked in yet :/
GLib libvirt
g_autofree VIR_AUTOFREE g_autoptr VIR_AUTOPTR g_auto VIR_AUTOCLEAR
For what it's worth, I think VIR_AUTOCLEAR is a much better name than g_auto :)
In GLib you are using them like this:
g_autofree char *string = NULL; g_autoptr(virDomain) dom = NULL; g_auto(virDomain) dom = { 0 };
So yes it would require to introduce a lot of typedefs for basic types and that is not worth it.
I'm not sure we would need so many typedefs, but there would certainly be a lot of churn involved. Personally, I'm not so sure it wouldn't be worth the effort, but it's definitely something that we can experiment with it at a later time instead of holding up what's already a pretty significant improvement.
In libvirt we would have:
VIR_AUTOFREE char *string = NULL; VIR_AUTOPTR(virDomainPtr) dom = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
If you notice the difference, in libvirt we can use virDomainPtr directly because we have these typedefs, in GLib macro G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
While I'm not a fan of our *Ptr typedefs in general, I guess this time I'm glad we have them because VIR_AUTOPTR() doesn't hide the fact that what you're declaring is a pointer; that is, the macro argument is also exactly the type of the variable. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
However, I realize it might not be possible to register free functions for a native type without having to introduce something like
typedef char * virString;
thus causing massive churn. How does GLib deal with that?
If you would look into GLib documentation you would see that this design basically copies the one in GLib:
Sorry, I should have looked up the documentation and implementation before asking silly questions. Guess the morning coffee hadn't quite kicked in yet :/
GLib libvirt
g_autofree VIR_AUTOFREE g_autoptr VIR_AUTOPTR g_auto VIR_AUTOCLEAR
For what it's worth, I think VIR_AUTOCLEAR is a much better name than g_auto :)
In GLib you are using them like this:
g_autofree char *string = NULL; g_autoptr(virDomain) dom = NULL; g_auto(virDomain) dom = { 0 };
So yes it would require to introduce a lot of typedefs for basic types and that is not worth it.
I'm not sure we would need so many typedefs, but there would certainly be a lot of churn involved.
Personally, I'm not so sure it wouldn't be worth the effort, but it's definitely something that we can experiment with it at a later time instead of holding up what's already a pretty significant improvement.
In libvirt we would have:
VIR_AUTOFREE char *string = NULL; VIR_AUTOPTR(virDomainPtr) dom = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
If you notice the difference, in libvirt we can use virDomainPtr directly because we have these typedefs, in GLib macro G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
While I'm not a fan of our *Ptr typedefs in general, I guess this time I'm glad we have them because VIR_AUTOPTR() doesn't hide the fact that what you're declaring is a pointer; that is, the macro argument is also exactly the type of the variable.
So let's make a summary of how it could look like: VIR_AUTOFREE(char *) string = NULL; VIR_AUTOPTR(virDomainPtr) vm = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 }; VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree); VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear); Note: don't take the types and function names as something that actually exists and be used like that, it's just an example to show how it would work :). Pavel
On 25 May 2018 at 16:20, Pavel Hrdina <phrdina@redhat.com> wrote:
On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
However, I realize it might not be possible to register free functions for a native type without having to introduce something like
typedef char * virString;
thus causing massive churn. How does GLib deal with that?
If you would look into GLib documentation you would see that this design basically copies the one in GLib:
Sorry, I should have looked up the documentation and implementation before asking silly questions. Guess the morning coffee hadn't quite kicked in yet :/
GLib libvirt
g_autofree VIR_AUTOFREE g_autoptr VIR_AUTOPTR g_auto VIR_AUTOCLEAR
For what it's worth, I think VIR_AUTOCLEAR is a much better name than g_auto :)
In GLib you are using them like this:
g_autofree char *string = NULL; g_autoptr(virDomain) dom = NULL; g_auto(virDomain) dom = { 0 };
So yes it would require to introduce a lot of typedefs for basic types and that is not worth it.
I'm not sure we would need so many typedefs, but there would certainly be a lot of churn involved.
Personally, I'm not so sure it wouldn't be worth the effort, but it's definitely something that we can experiment with it at a later time instead of holding up what's already a pretty significant improvement.
In libvirt we would have:
VIR_AUTOFREE char *string = NULL; VIR_AUTOPTR(virDomainPtr) dom = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
If you notice the difference, in libvirt we can use virDomainPtr directly because we have these typedefs, in GLib macro G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
While I'm not a fan of our *Ptr typedefs in general, I guess this time I'm glad we have them because VIR_AUTOPTR() doesn't hide the fact that what you're declaring is a pointer; that is, the macro argument is also exactly the type of the variable.
So let's make a summary of how it could look like:
VIR_AUTOFREE(char *) string = NULL; VIR_AUTOPTR(virDomainPtr) vm = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree); VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
Do we define new functions for freeing/clearing, because that is what VIR_DEFINE_AUTOFREE_FUNC seems to do. This is what new macros will look like: # define _VIR_TYPE_PTR(type) type##Ptr # define _VIR_ATTR_AUTOFREE_PTR(type) __attribute__((cleanup(type##Free))) # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close))) # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean))) # define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type) The problem is that our vir*Free functions take on vir*Ptr as the parameter and not vir*Ptr * (pointer to it). For example, instead of: void virArpTableFree(virArpTablePtr table); we would need: void virArpTableFree(virArpTablePtr *table); if we declare something like: VIR_AUTOFREE_PTR(virArpTable) table = NULL; Also, I tried to add a new function: void virArpTablePtrFree(virArpTablePtr *table) { size_t i; if (!*table) return; for (i = 0; i < (*table)->n; i++) { VIR_FREE((*table)->t[i].ipaddr); VIR_FREE((*table)->t[i].mac); } VIR_FREE((*table)->t); VIR_FREE((*table)); VIR_FREE(table); } but I am getting the errors: *** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-virbuftest': free(): invalid pointer: 0x00007ffc7be60d48 *** ... *** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-commandtest': free(): invalid pointer: 0x00007fff727583fc *** ... I am not quite sure how to debug this. Am I missing something basic?
Note: don't take the types and function names as something that actually exists and be used like that, it's just an example to show how it would work :).
Pavel
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
On 25 May 2018 at 16:20, Pavel Hrdina <phrdina@redhat.com> wrote:
On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
However, I realize it might not be possible to register free functions for a native type without having to introduce something like
typedef char * virString;
thus causing massive churn. How does GLib deal with that?
If you would look into GLib documentation you would see that this design basically copies the one in GLib:
Sorry, I should have looked up the documentation and implementation before asking silly questions. Guess the morning coffee hadn't quite kicked in yet :/
GLib libvirt
g_autofree VIR_AUTOFREE g_autoptr VIR_AUTOPTR g_auto VIR_AUTOCLEAR
For what it's worth, I think VIR_AUTOCLEAR is a much better name than g_auto :)
In GLib you are using them like this:
g_autofree char *string = NULL; g_autoptr(virDomain) dom = NULL; g_auto(virDomain) dom = { 0 };
So yes it would require to introduce a lot of typedefs for basic types and that is not worth it.
I'm not sure we would need so many typedefs, but there would certainly be a lot of churn involved.
Personally, I'm not so sure it wouldn't be worth the effort, but it's definitely something that we can experiment with it at a later time instead of holding up what's already a pretty significant improvement.
In libvirt we would have:
VIR_AUTOFREE char *string = NULL; VIR_AUTOPTR(virDomainPtr) dom = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
If you notice the difference, in libvirt we can use virDomainPtr directly because we have these typedefs, in GLib macro G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
While I'm not a fan of our *Ptr typedefs in general, I guess this time I'm glad we have them because VIR_AUTOPTR() doesn't hide the fact that what you're declaring is a pointer; that is, the macro argument is also exactly the type of the variable.
So let's make a summary of how it could look like:
VIR_AUTOFREE(char *) string = NULL; VIR_AUTOPTR(virDomainPtr) vm = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree); VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
Do we define new functions for freeing/clearing, because that is what VIR_DEFINE_AUTOFREE_FUNC seems to do.
This is what new macros will look like:
# define _VIR_TYPE_PTR(type) type##Ptr
# define _VIR_ATTR_AUTOFREE_PTR(type) __attribute__((cleanup(type##Free))) # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close))) # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
# define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type)
The problem is that our vir*Free functions take on vir*Ptr as the parameter and not vir*Ptr * (pointer to it).
For example, instead of: void virArpTableFree(virArpTablePtr table);
we would need: void virArpTableFree(virArpTablePtr *table);
Hmm, so gcc claims the cleanup attribute to take a function which is required to take a pointer to the type which it already is, it's just typedef'd, I haven't tried this myself but do you get a compiler error if you try it with the existing function, i.e. the one which doesn't mention the explicit '*' to signal a pointer type? If so and we can't use the our typedef'd Ptr type directly, then we'll need to switch all the free callback signatures to a plain virSomeType *arg rather than trying to pass a double pointer which is just unnecessary and ugly.
if we declare something like: VIR_AUTOFREE_PTR(virArpTable) table = NULL;
Also, I tried to add a new function: void virArpTablePtrFree(virArpTablePtr *table) { size_t i;
if (!*table) return;
for (i = 0; i < (*table)->n; i++) { VIR_FREE((*table)->t[i].ipaddr); VIR_FREE((*table)->t[i].mac); } VIR_FREE((*table)->t); VIR_FREE((*table)); VIR_FREE(table);
My honest guess would be ^this you're trying to free the double pointer itself which you didn't and you're also not supposed to allocate. Erik
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
On 25 May 2018 at 16:20, Pavel Hrdina <phrdina@redhat.com> wrote:
On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
However, I realize it might not be possible to register free functions for a native type without having to introduce something like
typedef char * virString;
thus causing massive churn. How does GLib deal with that?
If you would look into GLib documentation you would see that this design basically copies the one in GLib:
Sorry, I should have looked up the documentation and implementation before asking silly questions. Guess the morning coffee hadn't quite kicked in yet :/
GLib libvirt
g_autofree VIR_AUTOFREE g_autoptr VIR_AUTOPTR g_auto VIR_AUTOCLEAR
For what it's worth, I think VIR_AUTOCLEAR is a much better name than g_auto :)
In GLib you are using them like this:
g_autofree char *string = NULL; g_autoptr(virDomain) dom = NULL; g_auto(virDomain) dom = { 0 };
So yes it would require to introduce a lot of typedefs for basic types and that is not worth it.
I'm not sure we would need so many typedefs, but there would certainly be a lot of churn involved.
Personally, I'm not so sure it wouldn't be worth the effort, but it's definitely something that we can experiment with it at a later time instead of holding up what's already a pretty significant improvement.
In libvirt we would have:
VIR_AUTOFREE char *string = NULL; VIR_AUTOPTR(virDomainPtr) dom = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
If you notice the difference, in libvirt we can use virDomainPtr directly because we have these typedefs, in GLib macro G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
While I'm not a fan of our *Ptr typedefs in general, I guess this time I'm glad we have them because VIR_AUTOPTR() doesn't hide the fact that what you're declaring is a pointer; that is, the macro argument is also exactly the type of the variable.
So let's make a summary of how it could look like:
VIR_AUTOFREE(char *) string = NULL; VIR_AUTOPTR(virDomainPtr) vm = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree); VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
Do we define new functions for freeing/clearing, because that is what VIR_DEFINE_AUTOFREE_FUNC seems to do.
This is what new macros will look like:
# define _VIR_TYPE_PTR(type) type##Ptr
# define _VIR_ATTR_AUTOFREE_PTR(type) __attribute__((cleanup(type##Free))) # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close))) # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
# define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type)
NACK to this, please look few lines above how the macros should be named and which macros we would like to have implemented. There is no need to have the _VIR* helper macros and you need to implement the VIR_DEFINE_* macros.
The problem is that our vir*Free functions take on vir*Ptr as the parameter and not vir*Ptr * (pointer to it).
The problem is only with your current implementation, the original design should not have this issue. The part of VIR_DEFINE_* macros is definition of new wrapper function for the cleanup function which means that our existing free functions are not used directly. GLib version of the define macro: #define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \ typedef TypeName *_GLIB_AUTOPTR_TYPENAME(TypeName); \ G_GNUC_BEGIN_IGNORE_DEPRECATIONS \ static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) \ { \ if (*_ptr) \ (func) (*_ptr); \ } \ G_GNUC_END_IGNORE_DEPRECATIONS Obviously we don't need the "typedef" line and the DEPRECATIONS macros. Pavel
On 28 May 2018 at 13:54, Pavel Hrdina <phrdina@redhat.com> wrote:
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
On 25 May 2018 at 16:20, Pavel Hrdina <phrdina@redhat.com> wrote:
On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
However, I realize it might not be possible to register free functions for a native type without having to introduce something like
typedef char * virString;
thus causing massive churn. How does GLib deal with that?
If you would look into GLib documentation you would see that this design basically copies the one in GLib:
Sorry, I should have looked up the documentation and implementation before asking silly questions. Guess the morning coffee hadn't quite kicked in yet :/
GLib libvirt
g_autofree VIR_AUTOFREE g_autoptr VIR_AUTOPTR g_auto VIR_AUTOCLEAR
For what it's worth, I think VIR_AUTOCLEAR is a much better name than g_auto :)
In GLib you are using them like this:
g_autofree char *string = NULL; g_autoptr(virDomain) dom = NULL; g_auto(virDomain) dom = { 0 };
So yes it would require to introduce a lot of typedefs for basic types and that is not worth it.
I'm not sure we would need so many typedefs, but there would certainly be a lot of churn involved.
Personally, I'm not so sure it wouldn't be worth the effort, but it's definitely something that we can experiment with it at a later time instead of holding up what's already a pretty significant improvement.
In libvirt we would have:
VIR_AUTOFREE char *string = NULL; VIR_AUTOPTR(virDomainPtr) dom = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
If you notice the difference, in libvirt we can use virDomainPtr directly because we have these typedefs, in GLib macro G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
While I'm not a fan of our *Ptr typedefs in general, I guess this time I'm glad we have them because VIR_AUTOPTR() doesn't hide the fact that what you're declaring is a pointer; that is, the macro argument is also exactly the type of the variable.
So let's make a summary of how it could look like:
VIR_AUTOFREE(char *) string = NULL; VIR_AUTOPTR(virDomainPtr) vm = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree); VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
Do we define new functions for freeing/clearing, because that is what VIR_DEFINE_AUTOFREE_FUNC seems to do.
This is what new macros will look like:
# define _VIR_TYPE_PTR(type) type##Ptr
# define _VIR_ATTR_AUTOFREE_PTR(type) __attribute__((cleanup(type##Free))) # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close))) # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
# define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type)
NACK to this, please look few lines above how the macros should be named and which macros we would like to have implemented.
There is no need to have the _VIR* helper macros and you need to implement the VIR_DEFINE_* macros.
The problem is that our vir*Free functions take on vir*Ptr as the parameter and not vir*Ptr * (pointer to it).
The problem is only with your current implementation, the original design should not have this issue.
The part of VIR_DEFINE_* macros is definition of new wrapper function for the cleanup function which means that our existing free functions are not used directly.
GLib version of the define macro:
#define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \ typedef TypeName *_GLIB_AUTOPTR_TYPENAME(TypeName); \ G_GNUC_BEGIN_IGNORE_DEPRECATIONS \ static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) \ { \ if (*_ptr) \ (func) (*_ptr); \ } \ G_GNUC_END_IGNORE_DEPRECATIONS
Obviously we don't need the "typedef" line and the DEPRECATIONS macros.
So, using the discussed design, I have added the following lines: # define VIR_AUTOPTR_FUNC_NAME(type) type##Free # define VIR_AUTOCLEAR_FUNC_NAME(type) type##Clear # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \ static inline void VIR_AUTOPTR_FUNC_NAME(type) (type *_ptr) \ { \ if (*_ptr) \ (func) (*_ptr); \ } \ # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \ static inline void VIR_AUTOCLEAR_FUNC_NAME(type) (type *_ptr) \ { \ if (*_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 We need to add something like this for each type to autofree: VIR_DEFINE_AUTOFREE_FUNC(virArpTablePtr, virArpTableFree) Then we would use it like: VIR_AUTOFREEE(void *) nlData = NULL; VIR_AUTOPTR(virArpTablePtr) table = NULL; Does this seem fine? Also, I am still getting the errors by doing the above: ... *** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-virbuftest': free(): invalid pointer: 0x00007ffdd073a208 *** ... *** Error in `/home/skrtbhtngr/libvirt/tools/.libs/lt-virsh': free(): invalid pointer: 0x00007fc42349dcf9 *** ... I am not changing or adding a new function (beside the one defined in macro), just using original virArpTableFree. Also, since I am defining these in viralloc.h, we would need to include ALL header files where *Ptr typedefs are made. We can do this in three ways: - Include all the needed header files in viralloc.h and invoke all VIR_DEFINE_AUTOFREE_FUNC macros in viralloc.h itself. - We make a separate file for VIR_DEFINE_AUTOFREE_FUNC invocations where all the required *Ptr headers can be included. Then directly include that new header file in all the relevant .c files. - We invoke VIR_DEFINE_AUTOFREE_FUNC in respective header files, for example, invoke: VIR_DEFINE_AUTOFREE_FUNC(virArpTablePtr, virArpTableFree) in virarptable.h, and also include viralloc.h in it. Then, we do not need extra includes in .c files.
On Mon, May 28, 2018 at 09:40:41PM +0530, Sukrit Bhatnagar wrote:
On 28 May 2018 at 13:54, Pavel Hrdina <phrdina@redhat.com> wrote:
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
On 25 May 2018 at 16:20, Pavel Hrdina <phrdina@redhat.com> wrote:
On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote: > However, I realize it might not be possible to register free > functions for a native type without having to introduce something > like > > typedef char * virString; > > thus causing massive churn. How does GLib deal with that?
If you would look into GLib documentation you would see that this design basically copies the one in GLib:
Sorry, I should have looked up the documentation and implementation before asking silly questions. Guess the morning coffee hadn't quite kicked in yet :/
GLib libvirt
g_autofree VIR_AUTOFREE g_autoptr VIR_AUTOPTR g_auto VIR_AUTOCLEAR
For what it's worth, I think VIR_AUTOCLEAR is a much better name than g_auto :)
In GLib you are using them like this:
g_autofree char *string = NULL; g_autoptr(virDomain) dom = NULL; g_auto(virDomain) dom = { 0 };
So yes it would require to introduce a lot of typedefs for basic types and that is not worth it.
I'm not sure we would need so many typedefs, but there would certainly be a lot of churn involved.
Personally, I'm not so sure it wouldn't be worth the effort, but it's definitely something that we can experiment with it at a later time instead of holding up what's already a pretty significant improvement.
In libvirt we would have:
VIR_AUTOFREE char *string = NULL; VIR_AUTOPTR(virDomainPtr) dom = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
If you notice the difference, in libvirt we can use virDomainPtr directly because we have these typedefs, in GLib macro G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
While I'm not a fan of our *Ptr typedefs in general, I guess this time I'm glad we have them because VIR_AUTOPTR() doesn't hide the fact that what you're declaring is a pointer; that is, the macro argument is also exactly the type of the variable.
So let's make a summary of how it could look like:
VIR_AUTOFREE(char *) string = NULL; VIR_AUTOPTR(virDomainPtr) vm = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree); VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
Do we define new functions for freeing/clearing, because that is what VIR_DEFINE_AUTOFREE_FUNC seems to do.
This is what new macros will look like:
# define _VIR_TYPE_PTR(type) type##Ptr
# define _VIR_ATTR_AUTOFREE_PTR(type) __attribute__((cleanup(type##Free))) # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close))) # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
# define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type)
NACK to this, please look few lines above how the macros should be named and which macros we would like to have implemented.
There is no need to have the _VIR* helper macros and you need to implement the VIR_DEFINE_* macros.
The problem is that our vir*Free functions take on vir*Ptr as the parameter and not vir*Ptr * (pointer to it).
The problem is only with your current implementation, the original design should not have this issue.
The part of VIR_DEFINE_* macros is definition of new wrapper function for the cleanup function which means that our existing free functions are not used directly.
GLib version of the define macro:
#define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \ typedef TypeName *_GLIB_AUTOPTR_TYPENAME(TypeName); \ G_GNUC_BEGIN_IGNORE_DEPRECATIONS \ static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) \ { \ if (*_ptr) \ (func) (*_ptr); \ } \ G_GNUC_END_IGNORE_DEPRECATIONS
Obviously we don't need the "typedef" line and the DEPRECATIONS macros.
So, using the discussed design, I have added the following lines:
# define VIR_AUTOPTR_FUNC_NAME(type) type##Free # define VIR_AUTOCLEAR_FUNC_NAME(type) type##Clear
I would suggest to make the generated function name more unique and not that similar to our existing functions. For example: virAutoPtr##type virAutoClear##type
# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \ static inline void VIR_AUTOPTR_FUNC_NAME(type) (type *_ptr) \ { \ if (*_ptr) \ (func) (*_ptr); \
Theoretically the if is not required because our existing free functions already check the given pointer whether it's null or not. Also remove the extra space in (func) (*_ptr), that is GLib code-style. In libvirt we don't put extra space after function name.
} \
# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \ static inline void VIR_AUTOCLEAR_FUNC_NAME(type) (type *_ptr) \ { \ if (*_ptr) \ (func) (*_ptr); \
Here the if is not desired at all, usually the *_ptr would be directly some structure so we need to pass the pointer itself: (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
We need to add something like this for each type to autofree: VIR_DEFINE_AUTOFREE_FUNC(virArpTablePtr, virArpTableFree)
That's correct, ideally this call should be together with the free function in the same header file.
Then we would use it like: VIR_AUTOFREEE(void *) nlData = NULL; VIR_AUTOPTR(virArpTablePtr) table = NULL;
Does this seem fine?
Also, I am still getting the errors by doing the above: ... *** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-virbuftest': free(): invalid pointer: 0x00007ffdd073a208 *** ... *** Error in `/home/skrtbhtngr/libvirt/tools/.libs/lt-virsh': free(): invalid pointer: 0x00007fc42349dcf9 *** ...
This is strange and seems to be unrelated as you've already confirmed
I am not changing or adding a new function (beside the one defined in macro), just using original virArpTableFree.
Also, since I am defining these in viralloc.h, we would need to include ALL header files where *Ptr typedefs are made.
We can do this in three ways: - Include all the needed header files in viralloc.h and invoke all VIR_DEFINE_AUTOFREE_FUNC macros in viralloc.h itself. - We make a separate file for VIR_DEFINE_AUTOFREE_FUNC invocations where all the required *Ptr headers can be included. Then directly include that new header file in all the relevant .c files. - We invoke VIR_DEFINE_AUTOFREE_FUNC in respective header files, for example, invoke:
VIR_DEFINE_AUTOFREE_FUNC(virArpTablePtr, virArpTableFree)
in virarptable.h, and also include viralloc.h in it. Then, we do not need extra includes in .c files.
As I've already noted, this is the only solution, the other two solutions are really bad :). Anyway, with all of these notes it would be nice to see some patches posted to this list so everyone can see the resulting code. Pavel
On 05/28/2018 09:34 AM, Sukrit Bhatnagar wrote:
This is what new macros will look like:
# define _VIR_TYPE_PTR(type) type##Ptr
# define _VIR_ATTR_AUTOFREE_PTR(type) __attribute__((cleanup(type##Free))) # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close))) # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
# define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type)
The problem is that our vir*Free functions take on vir*Ptr as the parameter and not vir*Ptr * (pointer to it).
For example, instead of: void virArpTableFree(virArpTablePtr table);
we would need: void virArpTableFree(virArpTablePtr *table);
if we declare something like: VIR_AUTOFREE_PTR(virArpTable) table = NULL;
Also, I tried to add a new function: void virArpTablePtrFree(virArpTablePtr *table) { size_t i;
if (!*table) return;
for (i = 0; i < (*table)->n; i++) { VIR_FREE((*table)->t[i].ipaddr); VIR_FREE((*table)->t[i].mac); } VIR_FREE((*table)->t); VIR_FREE((*table)); VIR_FREE(table);
As Erik pointed out, this last VIR_FREE(table) looks fishy. However, do you have patch that I can apply and reproduce? Michal
On 29 May 2018 at 10:36, Michal Privoznik <mprivozn@redhat.com> wrote:
On 05/28/2018 09:34 AM, Sukrit Bhatnagar wrote:
This is what new macros will look like:
# define _VIR_TYPE_PTR(type) type##Ptr
# define _VIR_ATTR_AUTOFREE_PTR(type) __attribute__((cleanup(type##Free))) # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close))) # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
# define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type)
The problem is that our vir*Free functions take on vir*Ptr as the parameter and not vir*Ptr * (pointer to it).
For example, instead of: void virArpTableFree(virArpTablePtr table);
we would need: void virArpTableFree(virArpTablePtr *table);
if we declare something like: VIR_AUTOFREE_PTR(virArpTable) table = NULL;
Also, I tried to add a new function: void virArpTablePtrFree(virArpTablePtr *table) { size_t i;
if (!*table) return;
for (i = 0; i < (*table)->n; i++) { VIR_FREE((*table)->t[i].ipaddr); VIR_FREE((*table)->t[i].mac); } VIR_FREE((*table)->t); VIR_FREE((*table)); VIR_FREE(table);
As Erik pointed out, this last VIR_FREE(table) looks fishy. However, do you have patch that I can apply and reproduce?
When I was using the above function, even without the last VIR_FREE(table), I got the same error. I am now using the original virArpTableFree function, not this. The error is resolved for now. It seems like some other files I modified were creating trouble. I reset them.
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
On 25 May 2018 at 16:20, Pavel Hrdina <phrdina@redhat.com> wrote:
On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
However, I realize it might not be possible to register free functions for a native type without having to introduce something like
typedef char * virString;
thus causing massive churn. How does GLib deal with that?
If you would look into GLib documentation you would see that this design basically copies the one in GLib:
Sorry, I should have looked up the documentation and implementation before asking silly questions. Guess the morning coffee hadn't quite kicked in yet :/
GLib libvirt
g_autofree VIR_AUTOFREE g_autoptr VIR_AUTOPTR g_auto VIR_AUTOCLEAR
For what it's worth, I think VIR_AUTOCLEAR is a much better name than g_auto :)
In GLib you are using them like this:
g_autofree char *string = NULL; g_autoptr(virDomain) dom = NULL; g_auto(virDomain) dom = { 0 };
So yes it would require to introduce a lot of typedefs for basic types and that is not worth it.
I'm not sure we would need so many typedefs, but there would certainly be a lot of churn involved.
Personally, I'm not so sure it wouldn't be worth the effort, but it's definitely something that we can experiment with it at a later time instead of holding up what's already a pretty significant improvement.
In libvirt we would have:
VIR_AUTOFREE char *string = NULL; VIR_AUTOPTR(virDomainPtr) dom = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
If you notice the difference, in libvirt we can use virDomainPtr directly because we have these typedefs, in GLib macro G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
While I'm not a fan of our *Ptr typedefs in general, I guess this time I'm glad we have them because VIR_AUTOPTR() doesn't hide the fact that what you're declaring is a pointer; that is, the macro argument is also exactly the type of the variable.
So let's make a summary of how it could look like:
VIR_AUTOFREE(char *) string = NULL; VIR_AUTOPTR(virDomainPtr) vm = NULL; VIR_AUTOCLEAR(virDomain) dom = { 0 };
VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree); VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
Do we define new functions for freeing/clearing, because that is what VIR_DEFINE_AUTOFREE_FUNC seems to do.
This is what new macros will look like:
# define _VIR_TYPE_PTR(type) type##Ptr
# define _VIR_ATTR_AUTOFREE_PTR(type) __attribute__((cleanup(type##Free))) # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close))) # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
# define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type)
The problem is that our vir*Free functions take on vir*Ptr as the parameter and not vir*Ptr * (pointer to it).
For example, instead of: void virArpTableFree(virArpTablePtr table);
we would need: void virArpTableFree(virArpTablePtr *table);
This is actually a *good* thing. Passing in 'Ptr *' allows the virXXXFree function to set the pointer to NULL after free'ing it which prevents a double-free. If we need to change these free functions, that's a worthwhile improvement in general IMHO. 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 :|
participants (7)
-
Andrea Bolognani -
Daniel P. Berrangé -
Erik Skultety -
Michal Privoznik -
Pavel Hrdina -
Peter Krempa -
Sukrit Bhatnagar