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

[Erik Skultety]

On Mon, Jun 18, 2018 at 11:52:04AM +0100, Daniel P. Berrangé wrote:
> 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

>From security POV, I agree that the free functions should accept a double
pointer. However, in terms of GSoC and the time schedule, I think that having
the macros above is a nice stepping stone towards to long term goal to convert
our free functions to accept double pointers, besides, as you pointed out
below, the macros (at least the AUTOPTR_FUNC) makes sense with external types
and I like that we'd have a almost universal approach here. Even with the
macros, we don't lose anything (because they're straightforward and that's
important), quite the opposite, it's progress compared to the current status
quo.

>
> > # 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;

Andrea already pointed it out in his reply, but I don't like this
"simplification" either, the syntax is kinda obfuscating.

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

Yeah, looking back at when I came up with the idea, it is indeed unnecessarily
hacky. Having another macro for this purpose is an option, although Pavel had
some concerns that if we do that, people might misuse it every time they don't
know what the proper usage or approach is in that the type name and the
corresponding free function name should match. To me, that sounds like a
reviewer's job to spot. On the other hand, I think that the virStringList type
would truly be an exception here as it's unlikely that we'd be creating an
integer pointer-based list with everything else already being a compound type.
So, I'm still ambivalent here and I'd be fine with both having either an extra
macro or an extra typedef.


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

As I noted above, ^this specific example is rather unreadable and again we
should stick to the consistent convention MACRO(args) we agreed upon earlier.

In conclusion, this discussion has been going for a while with no apparent
progress, there have been a few proposals (it doesn't look there are going to
be any other ones) so now we should decide which direction we're going to head.
Overall, I like the approach in Pavel's summary as to me it's the cleanest
looking solutions of all, with the exception of the virStringList vs
VIR_AUTOFREE_FUNC issue, where I prefer the latter since it can happily coexist
with the rest, but having the former is not a show stopper to me either.

Thanks,
Erik