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.