|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/9] libxl: New event generation API
On Fri, 2012-01-13 at 19:25 +0000, Ian Jackson wrote:
> Replace the existing API for retrieving high-level events (events
> about domains, etc.) from libxl with a new one.
>
> This changes the definition and semantics of the `libxl_event'
> structure, and replaces the calls for obtaining information about
> domain death and disk eject events.
>
> This is an incompatible change, sorry. The alternative was to try to
> provide both the previous horrid API and the new one, and would also
> involve never using the name `libxl_event' for the new interface.
>
> The new "libxl_event" structure is blacklisted in the ocaml bindings
> for two reasons:
> - It has a field name "type" (which is a keyword in ocaml);
> the ocaml idl generator should massage this field name on
> output, to "type_" perhaps.
> - The ocaml idl generator does not support KeyedUnion.
>
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> ---
> tools/libxl/libxl.c | 329
> +++++++++++++++++++++++++++++-----------
> tools/libxl/libxl.h | 55 +------
> tools/libxl/libxl_event.c | 236 ++++++++++++++++++++++++++---
> tools/libxl/libxl_event.h | 183 ++++++++++++++++++++++-
> tools/libxl/libxl_internal.h | 77 +++++++++-
> tools/libxl/libxl_types.idl | 34 ++++-
> tools/libxl/xl_cmdimpl.c | 270 +++++++++++++++++++--------------
> tools/ocaml/libs/xl/genwrap.py | 1 +
> 8 files changed, 908 insertions(+), 277 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 413b684..19ff12c 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -95,6 +115,13 @@ int libxl_ctx_free(libxl_ctx *ctx)
>
> /* Deregister all libxl__ev_KINDs: */
>
> + free_disable_deaths(gc, &CTX->death_list);
> + free_disable_deaths(gc, &CTX->death_reported);
> +
> + libxl_evgen_disk_eject *eject;
> + while ((eject = LIBXL_LIST_FIRST(&CTX->disk_eject_evgens)))
> + libxl__evdisable_disk_eject(gc, eject);
Why a helper for deaths but not ejects?
[...]
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index ec66340..621a7cc 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
>
> /*
> diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
> index 63ef65e..0e83800 100644
> --- a/tools/libxl/libxl_event.h
> +++ b/tools/libxl/libxl_event.h
> +#define LIBXL_EVENTMASK_ALL (~(unsigned long)0)
> +
> +typedef int libxl_event_predicate(const libxl_event*, void *user);
> + /* Return value is 0 if the event is unwanted or non-0 if it is.
> + * Predicates are not allowed to fail.
> + */
> +
> +int libxl_event_check(libxl_ctx *ctx, libxl_event **event_r,
> + unsigned long typemask,
> + libxl_event_predicate *predicate, void
> *predicate_user);
> + /* Searches for an event, already-happened, which matches typemask
> + * and predicate. predicate==0 matches any event.
> + * libxl_event_check 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,
> + libxl_event_predicate *predicate, 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.
> + */
> +
> +void libxl_event_free(libxl_ctx *ctx, libxl_event *event);
> +
> +
> +/* Alternatively or additionally, the application may also use this: */
> +
> +typedef struct libxl_event_hooks {
> + uint64_t event_occurs_mask;
libxl_event_{wait,check} and LIBXL_EVENTMASK_ALL have an unsigned long
mask. Are they not the same set of bits?
[...]
> + * The user value is returned in the generated events and may be
> + * used by the caller for whatever it likes. The type ev_user is
> + * guaranteed to be an unsigned integer type which is at least
> + * as big as uint64_t and is also guaranteed to be big enough to
> + * contain any intptr_t value.
Does anything actually guarantee that sizeof(uint64_t) >
sizeof(intptr_t)? I'm sure it's true in practice and I'm happy to rely
on it. Just interested.
> + *[...]
> + * Applications should ensure that they eventually retrieve every
> + * event using libxl_event_check or libxl_event_wait, since events
> + * which occur but are not retreived by the application will be queued
retrieved
> + * inside libxl indefinitely. libxl_event_check/_wait may be O(n)
> + * where n is the number of queued events which do not match the
> + * criteria specified in the arguments to check/wait.
> + */
[...]
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 574dec7..a6dac79 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -395,3 +390,32 @@ libxl_sched_sedf = Struct("sched_sedf", [
> ("extratime", integer),
> ("weight", integer),
> ], dispose_fn=None)
> +
> +libxl_event_type = Enumeration("event_type", [
> + (1, "DOMAIN_SHUTDOWN"),
> + (2, "DOMAIN_DESTROY"),
> + (3, "DISK_EJECT"),
> + ])
> +
> +libxl_ev_user = UInt(64)
The other option here would be Builtin(...) and an entry in the builtin
table in the wrapper generator.
Arguably the idl could be improved by causing Number() to have a width
field. Currently it has a signedness and width is a property of UInt but
the latter could be pushed up the hierarchy.
You'd still end up with
FOO = Number("FOO", width=X)
which isn't really much better.
Or the ocaml generate could handle Number as the biggest int.
Hrm. None of that seems all that much better than what you have. Chalk
it up to potential future work.
> +libxl_ev_link = Builtin("ev_link", passby=PASS_BY_REFERENCE, private=True)
> +
> +libxl_event = Struct("event",[
> + ("link", libxl_ev_link,0),
This "0" == "const=False" which is the default. I don't think it is
necessary.
[...]
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8c30de1..e292bfc 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1702,92 +1729,106 @@ start:
> }
> LOG("Waiting for domain %s (domid %d) to die [pid %ld]",
> d_config.c_info.name, domid, (long)getpid());
[...]
> + ret = libxl_evenable_domain_death(ctx, domid, 0, &deathw);
> + if (ret) goto out;
>
[...]
> + if (!diskws) {
> + diskws = xmalloc(sizeof(*diskws) * d_config.num_disks);
I didn't see this getting freed on the exit path.
> + for (i = 0; i < d_config.num_disks; i++)
> + diskws[i] = NULL;
> + }
> + for (i = 0; i < d_config.num_disks; i++) {
> + ret = libxl_evenable_disk_eject(ctx, domid, d_config.disks[i].vdev,
> + 0, &diskws[i]);
> + if (ret) goto out;
> + }
This is all (I think) safe for num_disks == 0 but why waste the effort?
Incidentally we have libxl_device_disk.removable which might be an
opportunity to optimise? Assuming it is meaningful in that way. I think
in reality only emulated CD-ROM devices ever generate this event but
perhaps exposing that in the API overcomplicates things.
[...]
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |