Re: [Xen-devel] [PATCH 07/10] libxl: New API for providing OS events to libxl

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-01-11 at 17:32 +0000, Stefano Stabellini wrote:
> On Fri, 6 Jan 2012, Ian Jackson wrote:
> > +struct libxl__egc {
> > +    /* for event-generating functions only */
> > +    struct libxl__gc gc;
> > +};
> 
> [...]
> 
> >  /*
> > + * Calling context and GC for event-generating functions:
> > + *
> > + * These are for use by parts of libxl which directly or indirectly
> > + * call libxl__event_occurred.  These contain a gc but also a list of
> > + * deferred events.
> > + *
> > + * Most code in libxlshould not need to initialise their own egc.
> > + * Even functions which generate specific kinds of events don't need
> > + * to - rather, they will be passed an egc into their own callback
> > + * function and should just use the one they're given.
> > + *
> > + * A handy idiom for functions taking an egc is:
> > + *     libxl__gc *gc = &egc->gc;
> > + *
> > + * Functions using LIBXL__INIT_EGC may *not* generally be called from
> > + * within libxl, because libxl__egc_cleanup may call back into the
> > + * application.  This should be documented near the function
> > + * prototype(s) for callers of LIBXL__INIT_EGC and EGC_INIT.
> > + *
> > + * For the same reason libxl__egc_cleanup (or EGC_FREE) must be called
> > + * with the ctx *unlocked*.  So the right pattern has the EGC_...
> > + * macro calls on the outside of the CTX_... ones.
> > + */
> > +
> > +/* egc initialisation and destruction: */
> > +
> > +#define LIBXL_INIT_EGC(egc,ctx) do{             \
> > +        LIBXL_INIT_GC((egc).gc,ctx);            \
> > +        /* list of occurred events tbd */       \
> > +    } while(0)
> > +
> > +_hidden void libxl__egc_cleanup(libxl__egc *egc);
> > +
> > +/* convenience macros: */
> > +
> > +#define EGC_INIT(ctx)                       \
> > +    libxl__egc egc[1]; LIBXL_INIT_EGC(egc[0],ctx);      \
> > +    libxl__gc *gc = &egc->gc
> > +
> > +#define EGC_FREE           libxl__egc_cleanup(egc)
> 
> I still prefer the approach prototyped in
> http://marc.info/?l=xen-devel&m=132371797519877

I pointed out some issues with this in
http://marc.info/?l=xen-devel&m=132378496608354&w=2 which have gone
unanswered.

In particular error handling for the pthread_setspecific in
libxl__free_all seems pretty nasty to me since you would be introducing
a new failure path at the end of pretty much every libxl function. Worse
it is after the actual action of each function is otherwise complete so
you may end up having to undo stuff. I think it is going to be a lot of
work to fix all of those up properly.

Having frequently used pro/epilogue code (especially epilogue) which
cannot fail seems much simpler in general to me.

It occurred to me just now that having a different type of this type of
gc has the benefit that since libxl__event_occurred takes one it will
necessarily filter back up the call chain and help enforce/highlight the
requirement that functions which directly or indirectly call
libxl__event_occurred use an egc.

Ian.


> : avoid introducing
> restrictions on how libxl functions have to be called, unify egc and gc,
> make it possible to take the lock recursively and use the nested counter
> to figure out when to call the callbacks.
> It would make my head hurt less when I have to read/write this code,
> however others might react differently.
> 
> I don't think we'll ever get an agreement on this so I'll just step back
> and let other people decide what is the right approach.
> 
> Only one more thing: I would kindly ask to move all these event related
> functions to a different source file, to make it easier for people to
> understand which ones are different from the rest of the library.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel