Re: [libvirt] [GSoC] Code design for scalar and external types

On Mon, 18 Jun 2018 at 17:20, Andrea Bolognani <abologna(a)redhat.com&gt; 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.