On Mon, Jun 25, 2018 at 10:22:14AM +0200, Erik Skultety wrote:
> 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.
I have nothing else to add so ACK to all of that.
Pavel
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
