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

[Daniel P. Berrangé]

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