|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/3] RFC: libxl: API changes re event handling
I think it would be good to have the model validated by Jim (CC'd) since
I imagine that the libvirt backend will be one of the primary users of
the registration mechanism.
There's a lot of docs here, which is excellent, but I've only had two
cups of tea so far today so I only managed a fairly high-level think
about it all.
On Tue, 2011-07-12 at 19:37 +0100, Ian Jackson wrote:
> -/* events handling */
> +
> +
> +/*
> + * OS event handling - passing low-level OS events to libxl
> + *
> + * Event-driven programs must use these facilities to allow libxl
> + * to become aware of readability/writeability of file descriptors
> + * and the occurrence of timeouts.
> + *
> + * There are two approaches available. The first is appropriate for
> + * simple programs handling reasonably small numbers of domains:
> + *
> + * for (;;) {
> + * libxl_osevent_beforepoll(...)
> + * poll();
> + * libxl_osevent_afterpoll(...);
> + * for (;;) {
> + * r=libxl_event_check(...);
> + * if (r==LIBXL_NOT_READY) break;
> + * if (r) handle failure;
> + * do something with the event;
> + * }
> + * }
> + *
> + * The second approach uses libxl_osevent_register_hooks and is
> + * suitable for programs which are already using a callback-based
> + * event library.
An example of this style of usage would be handy too.
> + *
> + * An application may freely mix the two styles of interaction.
> + */
> +
> +int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> + struct poll *fds, int *timeout_upd);
> + /* app should provide beforepoll with some fds, and give number in
> *nfds_io.
> + * *nfds_io will in any case be updated according to how many we want.
> + * if space was sufficient, fills in fds[0..<new *nfds_io>]
> + * and updates *timeout_upd if needed and returns ok.
> + * if space was insufficient, fds[0..<old *nfds_io>] is undefined,
> + * *timeout_upd may or may not have been updated, returns ERROR_BUFERFULL.
> + */
It's not immediately obvious but I presume the user is entitled to
provide an fds etc which already contains the file descriptors it is
itself interested in and that this function will augment it.
Is there some way for a caller to determine how many entries in the
struct poll * the library will use?
Should we also have an interface suitable for users of select?
It is currently conventional in libxl.h to document a function before
it's prototype rather than after.
> +void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct poll
> *fds);
> + /* nfds and fds[0..nfds] must be from _beforepoll as modified by poll
> + */
What does this function do in practice?
> +int libxl_osevent_register_hooks(libxl_ctx *ctx, void *user,
> + int (*fd_register)(void *user, int fd, void **for_app_registration_out,
> + short events, void *for_libxl),
> [...]
> + void (*time_deregister)(void *user, void *for_app_registration_io,
> + void *for_libxl));
I think passing a pointer to a struct libxl_osevent_hooks would be
better than passing loads of fn pointers.
> + /* The application which calls register_fd_hooks promises to
> + * maintain a register of fds and timeouts that libxl is interested
> + * in,
It would probably be useful to provide some helper functions for
maintaining this register and for dispatching the foo_occurred things.
[...]
> typedef struct {
> /* event type */
> @@ -293,41 +410,136 @@ typedef struct {
> char *token;
> } libxl_event;
>
> -typedef struct {
> - char *path;
> - char *token;
> -} libxl_waiter;
> +int libxl_event_check(libxl_ctx *ctx, libxl_event *event_r,
> + unsigned long typemask,
> + int (*predicate)(const libxl_event*, void *user),
> + void *predicate_user);
I was a little confused for a while because this function is associated
with the libxl_osevent_*poll methods but the register_hooks stuff is
defined in the middle. I think it would be better to define both sets of
functions in contiguous and distinct blocks.
> + /* Searches for an event, already-happened, which matches typemask
> + * and predicate. predicate==0 matches any event. Returns the
> + * event, which must then later be freed by the caller using
> + * libxl_event_free.
> + *
> + * Returns ERROR_NOT_READY if no such event has happened.
> + */
>
> +int libxl_event_wait(libxl_ctx *ctx, libxl_event *event_r,
> + unsigned long typemask,
> + int (*predicate)(const libxl_event*, void *user),
> + void *predicate_user);
> + /* Like libxl_event_check but blocks if no suitable events are
> + * available, until some are. Uses
> + * libxl_osevent_beforepoll/_afterpoll so may be inefficient if very
> + * many domains are being handled by a single program.
> + */
[...]
> +int libxl_event_free(libxl_ctx, *ctx, libxl_event *event);
It looks as if the libxl_event passed to libxl_event_wait is owned by
the caller so it could use a local struct or manage allocation itself,
is that right?
In which case the autogenerated libxl_event_destroy is sufficient
instead of free?
> +
> +
> +/* Alternatively or additionally, the application may also use this: */
> +
> +void libxl_event_register_callbacks(libxl_ctx *ctx, void *user, uint64_t
> mask,
> + void (*event_occurs)(void *user, const libxl_event *event),
> + void (*disaster)(void *user, libxl_event_type type,
> + const char *msg, int errnoval));
> + /*
> + * Arranges that libxl will henceforth call event_occurs for any
> + * events whose type is set in mask, rather than queueing the event
> + * for retrieval by libxl_event_check/wait. Events whose bit is
> + * clear in mask are not affected.
Would it be useful to have separate callbacks for the different event
types?
Or perhaps just a helper function which can be registered here and takes
a dispatch table from the app (I suppose the app could build this
itself, but maybe it would be useful functionality).
> + *
> + * event becomes owned by the application and must be freed, either
> + * by event_occurs or later.
> + *
> + * event_occurs may be NULL if mask is 0.
> + *
> + * libxl_event_register_callback also provides a way for libxl to
> + * report to the application that there was a problem reporting
> + * events; this can occur due to lack of host memory during event
> + * handling, or other wholly unrecoverable errors from system calls
> + * made by libxl. This will not happen for frivolous reasons - only
> + * if the system, or the Xen components of it, are badly broken.
> + *
> + * msg and errnoval will describe the action that libxl was trying
> + * to do, and type specifies the type of libxl events which may be
> + * missing. type may be 0 in which case events of all types may be
> + * missing.
> + *
> + * disaster may be NULL. If it is, or if
> + * libxl_event_register_callbacks has not been called, errors of
> + * this kind are fatal to the entire application: libxl will print a
> + * message to stderr and call exit(-1).
> + *
> + * If disaster returns, it may be the case that some or all future
> + * libxl calls will return errors; likewise it may be the case that
> + * no more events (of the specified type, if applicable) can be
> + * produced. An application which supplies a disaster function
> + * should normally react either by exiting, or by (when it has
> + * returned to its main event loop) shutting down libxl with
> + * libxl_ctx_free and perhaps trying to restart it with
> + * libxl_ctx_init.
> + *
> + * Reentrancy: it IS permitted to call libxl from within
> + * event_occurs. It is NOT permitted to call libxl from within
> + * disaster.
> + *
> + * libxl_event_register_callbacks may be called as many times, with
> + * different parameters, as the application likes; the most recent
> + * call determines the libxl behaviour. There is only one mask.
> + */
>
> -/*
> - * Returns:
> - * - 0 if the domain is dead but there is no cleanup to be done. e.g
> - * because someone else has already done it.
> - * - 1 if the domain is dead and there is cleanup to be done.
> - *
> - * Can return error if the domain exists and is still running.
> - *
> - * *info will contain valid domain state iff 1 is returned. In
> - * particular if 1 is returned then info->shutdown_reason is
> - * guaranteed to be valid since by definition the domain is
> - * (shutdown||dying))
> - */
> -int libxl_event_get_domain_death_info(libxl_ctx *ctx, uint32_t domid,
> libxl_event *event, libxl_dominfo *info);
>
> -/*
> - * Returns true and fills *disk if the caller should eject the disk
> +/* Events are only generated if they have been requested.
> + * The following functions request the generation of specific events.
> */
> -int libxl_event_get_disk_eject_info(libxl_ctx *ctx, uint32_t domid,
> libxl_event *event, libxl_device_disk *disk);
> +
> +typedef struct libxl_awaiting_domain_death libxl_awaiting_domain_death;
> +int libxl_await_domain_death(libxl_ctx *ctx, uint32_t domid, uint64_t user,
> + libxl_awaiting_domain_death **waiter_out);
> +void libxl_noawait_domain_death(libxl_ctx *ctx, uint32_t domid,
> + libxl_awaiting_domain_death *waiter);
> + /* Generates DOMAIN_SHUTDOWN and DOMAIN_DESTROY events.
> + * A domain which is destroyed before it shuts down generates
> + * only a DESTROY event. */
> +
> +typedef struct libxl_awaiting_disk_eject libxl_awaiting_disk_eject;
Is this struct defined in the IDL? If yes then I think you get this for
free. If no then shouldn't it be?
> diff --git a/tools/libxl/libxl.idl b/tools/libxl/libxl.idl
> index 8fb883f..8b285a4 100644
> --- a/tools/libxl/libxl.idl
> +++ b/tools/libxl/libxl.idl
> @@ -73,11 +73,6 @@ libxl_action_on_shutdown =
> Enumeration("action_on_shutdown", [
> (6, "COREDUMP_RESTART"),
> ])
>
> -libxl_event_type = Enumeration("event_type", [
> - (1, "DOMAIN_DEATH"),
> - (2, "DISK_EJECT"),
> - ])
> -
> libxl_button = Enumeration("button", [
> (1, "POWER"),
> (2, "SLEEP"),
> @@ -371,3 +366,26 @@ libxl_sched_credit = Struct("sched_credit", [
> ("weight", integer),
> ("cap", integer),
> ], destructor_fn=None)
> +
> +libxl_event_type = Enumeration("event_type", [
> + (1, "DOMAIN_SHUTDOWN"),
> + (2, "DOMAIN_DESTROY"),
> + (3, "DISK_EJECT"),
> + ])
> +
> +libxl_event = Struct("event",[
> + ("domid", libxl_domid),
> + ("domuuid", libxl_uuid),
> + ("type", libxl_event_type),
> + ("for_user", uint64),
> + ("u", KeyedUnion(None,"type",
> + [("domain_shutdown", Struct(None, [
> + ("shutdown_reason", uint8)
> + ])),
> + ("domain_destroy", Struct()),
> + ("disk_eject", Struct(None, [
> + ("vdev", string)
> + ("disk", libxl_device_disk)
> + ])),
> + ]))])
It might be convenient to users if these structs were named so they can
dispatch to handle_domain_shutdown(struct libxl_event_domain_shutdown
*info) ?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |