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 :|