Re: [PATCH 10/23] libxl: events: Provide libxl__ev_evtchn*

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> This also involves providing a gc for the latter part of
> libxl_ctx_alloc.

Didn't you do that bit in an earlier patch? I guess you forgot to update
this message when you refactored.

> +static void evtchn_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
> +                               int fd, short events, short revents)
> +{
> +    EGC_GC;
> +    libxl__ev_evtchn *evev;
> +    int port, r, rc;

Should port be evtchn_port_or_error_t ? (from the use I don't think
plain evtchn_port_t would be valid)

> [...]
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 5d2e651..c519abc 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -197,6 +197,17 @@ struct libxl__ev_xswatch {
>      uint32_t counterval;
>  };
>  
> +typedef struct libxl__ev_evtchn libxl__ev_evtchn;
> +typedef void libxl__ev_evtchn_callback(libxl__egc *egc, libxl__ev_evtchn*);
> +struct libxl__ev_evtchn {
> +    /* caller must fill these in, and they must all remain valid */
> +    libxl__ev_evtchn_callback *callback;
> +    int port;

evtchn_port_t?

> +    /* remainder is private for libxl__ev_evtchn_... */
> +    bool waiting;
> +    LIBXL_LIST_ENTRY(libxl__ev_evtchn) entry;
> +};
> +
>  /*
>   * An entry in the watch_slots table is either:
>   *  1. an entry in the free list, ie NULL or pointer to next free list entry
> @@ -343,6 +354,10 @@ struct libxl__ctx {
>      uint32_t watch_counter; /* helps disambiguate slot reuse */
>      libxl__ev_fd watch_efd;
>  
> +    xc_evtchn *xce; /* for waiting use only libxl__ev_evtchn* */

The comment means "don't use directly, use libxl__ev-evtchn"?

Or does it imply that for uses other than waiting you may use it
directly?

> +    LIBXL_LIST_HEAD(, libxl__ev_evtchn) evtchns_waiting;

Are there any locking requirements relating to this list?

> +    libxl__ev_fd evtchn_efd;
> +
>      LIBXL_TAILQ_HEAD(libxl__evgen_domain_death_list, libxl_evgen_domain_death)
>          death_list /* sorted by domid */,
>          death_reported;
> @@ -748,6 +763,39 @@ static inline int libxl__ev_xswatch_isregistered(const libxl__ev_xswatch *xw)
>  
> 
>  /*
> + * The evtchn facility is one-shot per call t libxl__ev_evtchn_wait.

s/ t / to /

> + * You should call some suitable xc bind function on (or to obtain)
> + * the port, then libxl__ev_evtchn_wait.
> + *
> + * When the event is signaled then the callback will be made, once.
> + * Then you must call libxl__ev_evtchn_wait again, if desired.
> + *
> + * You must NOT call xc_evtchn_unmask.  wait will do that for you.
> + *
> + * Calling libxl__ev_evtchn_cancel will arrange for libxl to disregard
> + * future occurrences of event.  Both libxl__ev_evtchn_wait and
> + * libxl__ev_evtchn_cancel are idempotent.
> + *
> + * (Note of course that an event channel becomes signaled when it is
> + * first bound, so you will get one call to libxl__ev_evtchn_wait
> + * "right away"; unless you have won a very fast race, the condition
> + * you were waiting for won't exist yet so when you check for it
> + * you'll find you need to call wait again.)
> + *
> + * You must not wait on the same port twice at once (that is, with
> + * two separate libxl__ev_evtchn's).

Doing so would require per wait xce_handles? Worth avoiding.

> + */
> +_hidden int libxl__ev_evtchn_wait(libxl__gc*, libxl__ev_evtchn *evev);
> +_hidden void libxl__ev_evtchn_cancel(libxl__gc *gc, libxl__ev_evtchn *evev);
> +
> +static inline void libxl__ev_evtchn_init(libxl__ev_evtchn *evev)
> +                { evev->waiting = 0; }
> +static inline bool libxl__ev_evtchn_iswaiting(const libxl__ev_evtchn *evev)
> +                { return evev->waiting; }
> +
> +_hidden int libxl__ctx_evtchn_init(libxl__gc *gc); /* for libxl_ctx_alloc */
> +
> +/*
>   * For making subprocesses.  This is the only permitted mechanism for
>   * code in libxl to do so.
>   *