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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-01-06 at 20:35 +0000, Ian Jackson wrote:
> We provide a new set of functions and related structures
>   libxl_osevent_*
> which are to be used by event-driven applications to receive
> information from libxl about which fds libxl is interested in, and
> what timeouts libxl is waiting for, and to pass back to libxl
> information about which fds are readable/writeable etc., and which
> timeouts have occurred.  Ie, low-level events.
> 
> In this patch, this new machinery is still all unused.  Callers will
> appear in the next patch in the series, which introduces a new API for
> applications to receive high-level events about actual domains etc.
> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

I think I've mostly looked at this already so I only glanced through it
this time.

> [...]

> +int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> +                             struct pollfd *fds, int *timeout_upd,
> +                             struct timeval now);
> +  /* The caller should provide beforepoll with some space for libxl's
> +   * fds, and tell libxl how much space is available by setting *nfds_io.
> +   * fds points to the start of this space (and fds may be a pointer into
> +   * a larger array, for example, if the application has some fds of
> +   * its own that it is interested in).

I think you were going to move the comments (sorry).

> [...]
> @@ -733,6 +950,49 @@ libxl__device_model_version_running(libxl__gc *gc, 
> uint32_t domid);
> 
> 
>  /*
> + * 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.

Missing space.         ^

Will any function even need to initialise their own egc and/or is it
normally provided by infrastructure code? Otherwise when should a
function initialise their own egc? Is it simply that you know if you
need to initialise one because you are calling a function which takes
one?

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

You've provided lots of handy macros for other things like this, perhaps
EGC by comparison with CTX. Using GC seems better on the face of it but
since it will only work in a context with an egc that is actually
confusing.

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

Can we also enforce/check this with a flag in the ctx? I guess not since
multiple threads might call into libxl with the same ctx and each may
legitimately setup an egc.?

> + *
> + * 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)
> +
> +
> +/*
>   * Convenience macros.
>   */
> 
> --
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



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