|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 15/15] libxl: New event generation API
On Mon, 2011-12-05 at 18:10 +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.
>
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> ---
> tools/libxl/libxl.c | 302
> ++++++++++++++++++++++++++++++------------
> tools/libxl/libxl.h | 55 ++-------
> tools/libxl/libxl_event.c | 188 ++++++++++++++++++++++++---
> tools/libxl/libxl_event.h | 180 ++++++++++++++++++++++++-
> tools/libxl/libxl_internal.c | 6 +
> tools/libxl/libxl_internal.h | 81 +++++++++++-
> tools/libxl/libxl_types.idl | 35 ++++-
> tools/libxl/xl_cmdimpl.c | 270 ++++++++++++++++++++++----------------
> 8 files changed, 839 insertions(+), 278 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 58f280c..ba9293b 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -614,117 +631,173 @@ int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t
> domid, int req)
> return 0;
> }
>
> -int libxl_get_wait_fd(libxl_ctx *ctx, int *fd)
> -{
> - *fd = xs_fileno(ctx->xsh);
> - return 0;
> -}
> +static void domain_death_xswatch_callback(libxl__gc *gc, libxl__ev_xswatch
> *w,
> + const char *wpath, const char
> *epath) {
> + libxl_evgen_domain_death *evg;
> + uint32_t domid;
> + int rc;
Ugh, diff has made some very unhelpful choices about how to display this
change. Oh well. I've snipped all the - bits to try and make it easier
to follow.
[...]
> + CTX_LOCK;
[...]
> + uint32_t domid = libxl_get_stubdom_id(ctx, guest_domid);
> + evg = LIBXL_TAILQ_FIRST(&CTX->death_list);
> + if (!evg) goto out;
>
> + domid = evg->domid;
>
[...]
> + for (;;) {
> + int nentries = LIBXL_TAILQ_NEXT(evg, entry) ? 200 : 1;
> + xc_domaininfo_t domaininfos[nentries];
> + const xc_domaininfo_t *got = domaininfos, *gotend;
> +
> + rc = xc_domain_getinfolist(CTX->xch, domid, nentries, domaininfos);
> + if (rc == -1) {
> + LIBXL__EVENT_DISASTER(gc, "xc_domain_getinfolist failed while"
> + " processing @releaseDomain watch event",
> + errno, 0);
> goto out;
[...]
> + }
> + gotend = &domaininfos[rc];
> +
> + for (;;) {
> + if (!evg)
> + goto all_reported;
> +
> + if (!rc || got->domain > evg->domid) {
> + /* ie, the list doesn't contain evg->domid any more so
> + * the domain has been destroyed */
> + libxl_evgen_domain_death *evg_next;
> +
> + libxl_event *ev = NEW_EVENT(gc, DOMAIN_DESTROY, evg->domid);
> + if (!ev) goto out;
> +
> + libxl__event_occurred(gc, ev);
> +
> + evg->death_reported = 1;
> + evg_next = LIBXL_TAILQ_NEXT(evg, entry);
> + LIBXL_TAILQ_REMOVE(&CTX->death_list, evg, entry);
> + LIBXL_TAILQ_INSERT_HEAD(&CTX->death_reported, evg, entry);
> + evg = evg_next;
> +
> + continue;
> + }
> +
> + if (got == gotend)
> + break;
> +
> + if (got->domain < evg->domid) {
> + got++;
> + continue;
> + }
> +
> + assert(evg->domid == got->domain);
> +
> + if (!evg->shutdown_reported &&
> + (got->flags & XEN_DOMINF_shutdown)) {
> + libxl_event *ev = NEW_EVENT(gc, DOMAIN_SHUTDOWN,
> got->domain);
> + if (!ev) goto out;
> +
> + ev->u.domain_shutdown.shutdown_reason =
> + (got->flags >> XEN_DOMINF_shutdownshift) &
> + XEN_DOMINF_shutdownmask;
> + libxl__event_occurred(gc, ev);
> +
> + evg->shutdown_reported = 1;
> + }
> + evg = LIBXL_TAILQ_NEXT(evg, entry);
> + }
> +
> + assert(rc); /* rc==0 results in us eating all evgs and quitting */
> + domid = gotend[-1].domain;
> }
[...]
> + all_reported:
> + out:
> +
> + CTX_UNLOCK;
> }
>
[...]
> +int libxl_evenable_domain_death(libxl_ctx *ctx, uint32_t domid,
> + libxl_ev_user user, libxl_evgen_domain_death **evgen_out) {
> + GC_INIT(ctx);
> + libxl_evgen_domain_death *evg, *evg_search;
> + int rc;
> +
> + CTX_LOCK;
> +
> + evg = malloc(sizeof(*evg)); if (!evg) { rc = ERROR_NOMEM; goto out; }
CODING_STYLE?
> + memset(evg, 0, sizeof(*evg));
> + evg->domid = domid;
> + evg->user = user;
> +
> + LIBXL_TAILQ_INSERT_SORTED(&ctx->death_list, entry, evg, evg_search, ,
> + evg->domid > evg_search->domid);
> +
> + if (!libxl__ev_xswatch_isregistered(&ctx->death_watch)) {
> + rc = libxl__ev_xswatch_register(gc, &ctx->death_watch,
> + domain_death_xswatch_callback, "@releaseDomain");
> + if (rc) { libxl__evdisable_domain_death(gc, evg); goto out; }
> }
[...]
> + rc = 0;
>
[...]
> + out:
> + CTX_UNLOCK;
> + return rc;
> +};
>
[...]
> +void libxl__evdisable_domain_death(libxl__gc *gc,
> + libxl_evgen_domain_death *evg) {
> + CTX_LOCK;
>
[...]
> + if (!evg->death_reported)
> + LIBXL_TAILQ_REMOVE(&CTX->death_list, evg, entry);
> + else
> + LIBXL_TAILQ_REMOVE(&CTX->death_reported, evg, entry);
>
[...]
> + free(evg);
> +
> + if (!LIBXL_TAILQ_FIRST(&CTX->death_list) &&
> + libxl__ev_xswatch_isregistered(&CTX->death_watch))
> + libxl__ev_xswatch_deregister(gc, &CTX->death_watch);
>
[...]
> + CTX_UNLOCK;
> }
>
[...]
> +void libxl_evdisable_domain_death(libxl_ctx *ctx,
> + libxl_evgen_domain_death *evg) {
> GC_INIT(ctx);
[...]
> + libxl__evdisable_domain_death(gc, evg);
> + GC_FREE;
> +}
> +
> +static void disk_eject_xswatch_callback(libxl__gc *gc, libxl__ev_xswatch
> *watch,
> + const char *wpath, const char
> *epath) {
> + libxl_evgen_disk_eject *evg = (void*)watch;
> char *backend;
> char *value;
> char backend_type[BACKEND_STRING_SIZE+1];
>
[...]
> + value = libxl__xs_read(gc, XBT_NULL, wpath);
>
[...]
> + if (!value || strcmp(value, "eject"))
> + return;
> +
> + if (libxl__xs_write(gc, XBT_NULL, wpath, "")) {
> + LIBXL__EVENT_DISASTER(gc, "xs_write failed acknowledging eject",
> + errno, LIBXL_EVENT_TYPE_DISK_EJECT);
> + return;
> }
>
> - path = strdup(event->path);
[...]
> + libxl_event *ev = NEW_EVENT(gc, DISK_EJECT, evg->domid);
> + libxl_device_disk *disk = &ev->u.disk_eject.disk;
> +
> + backend = libxl__xs_read(gc, XBT_NULL,
> + libxl__sprintf(gc, "%.*s/backend",
> + (int)strlen(wpath)-6, wpath));
This pattern crops up a lot in libxl. I keep wondering if libxl__xs_read
shouldn't take a fmt string and varargs. Not sure how you would do that
in a way which allowed libxl__xs_write to have an orthogonal (and still
sane) interface though.
>
> sscanf(backend,
[...]
> + "/local/domain/%d/backend/%" TOSTRING(BACKEND_STRING_SIZE)
> + "[a-z]/%*d/%*d",
> + &disk->backend_domid, backend_type);
> if (!strcmp(backend_type, "tap") || !strcmp(backend_type, "vbd")) {
> disk->backend = LIBXL_DISK_BACKEND_TAP;
> } else if (!strcmp(backend_type, "qdisk")) {
> @@ -733,19 +806,72 @@ int libxl_event_get_disk_eject_info(libxl_ctx *ctx,
> uint32_t domid, libxl_event
> disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
> }
>
[...]
> + disk->pdev_path = strdup(""); /* xxx fixme malloc failure */
> disk->format = LIBXL_DISK_FORMAT_EMPTY;
> /* this value is returned to the user: do not free right away */
[...]
> + disk->vdev = xs_read(CTX->xsh, XBT_NULL,
> + libxl__sprintf(gc, "%s/dev", backend), NULL);
> disk->removable = 1;
> disk->readwrite = 0;
> disk->is_cdrom = 1;
>
[...]
> + libxl__event_occurred(gc, ev);
> +}
> +
[...]
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 654a5b0..17b15a6 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -53,7 +53,10 @@
> * A public function may be called from within libxl; the call
> * context initialisation macros will make sure that the internal
> * caller's context is reused (eg, so that the same xenstore
> - * transaction is used).
> + * transaction is used). But in-libxl callers of libxl public
> + * functions should note that any libxl public function may cause
> + * recursively reentry into libxl via the application's event
> + * callback hook.
> *
> * Public functions have names like libxl_foobar.
> *
> @@ -152,6 +155,8 @@ void libxl_key_value_list_dispose(libxl_key_value_list
> *kvl);
>
> typedef uint32_t libxl_hwcap[8];
>
> +typedef uint64_t libxl_ev_user;
> +
> typedef struct {
> uint32_t size; /* number of bytes in map */
> uint8_t *map;
> @@ -200,6 +205,9 @@ typedef struct {
> int v;
> } libxl_enum_string_table;
>
> +struct libxl_event;
> +typedef LIBXL_TAILQ_ENTRY(struct libxl_event) libxl_ev_link;
I think I made a comment on an earlier patch about why the list stuff
was exposed to the user. I guess I now have my answer.
[...]
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index 8d4dbf6..ec5c25e 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
[...]
> @@ -703,6 +713,152 @@ void libxl__event_disaster(libxl__gc *gc, const char
> *msg, int errnoval,
> }
>
> /*
> + * Event retrieval etc.
> + */
> +
> +void libxl_event_register_callbacks(libxl_ctx *ctx,
> + const libxl_event_hooks *hooks, void *user) {
> + ctx->event_hooks = hooks;
> + ctx->event_hooks_user = user;
Does this either need locking or a check that there are no events in
flight? Or is it invalid to change hooks once they are registered?
> +}
> +
> +void libxl__event_occurred(libxl__gc *gc, libxl_event *event) {
> + if (CTX->event_hooks &&
> + (CTX->event_hooks->event_occurs_mask & (1UL << event->type))) {
> + /* libxl__free_all will call the callback, just before exit
> + * from libxl. This helps avoid reentrancy bugs: parts of
> + * libxl that call libxl__event_occurred do not have to worry
> + * that libxl might be reentered at that point. */
> + LIBXL_TAILQ_INSERT_TAIL(&gc->occurred_for_callback, event, link);
> + return;
This return seem surplus to requirements.
> + } else {
> + LIBXL_TAILQ_INSERT_TAIL(&CTX->occurred, event, link);
> + }
> +}
> +
> +void libxl_event_free(libxl_ctx *ctx, libxl_event *event) {
> + libxl_event_dispose(event);
> + free(event);
> +}
> +
> +libxl_event *libxl__event_new(libxl__gc *gc,
> + libxl_event_type type, uint32_t domid) {
> + libxl_event *ev;
> +
> + ev = malloc(sizeof(*ev));
> + if (!ev) {
> + LIBXL__EVENT_DISASTER(gc, "allocate new event", errno, type);
> + return NULL;
> + }
> +
> + memset(ev, 0, sizeof(*ev));
> + ev->type = type;
> + ev->domid = domid;
> +
> + return ev;
> +}
> +
> +static int event_check_unlocked(libxl__gc *gc, libxl_event **event_r,
The *_unlocked functions are slightly confusing, since what it really
means is "caller must have taken the lock already" rather than "you can
call this unlocked". I'm not sure what would be better though.
> + unsigned long typemask,
> + libxl_event_predicate *pred, void
> *pred_user) {
> + libxl_event *ev;
> + int rc;
> +
> + LIBXL_TAILQ_FOREACH(ev, &CTX->occurred, link) {
> + if (!(typemask & (1UL << ev->type)))
> + continue;
> +
> + if (pred && !pred(ev, pred_user))
> + continue;
> +
> + /* got one! */
> + LIBXL_TAILQ_REMOVE(&CTX->occurred, ev, link);
> + *event_r = ev;
> + rc = 0;
> + goto out;
> + }
> + rc = ERROR_NOT_READY;
> +
> + out:
> + return rc;
> +}
> +
> +int libxl_event_check(libxl_ctx *ctx, libxl_event **event_r,
> + unsigned long typemask,
> + libxl_event_predicate *pred, void *pred_user) {
> + GC_INIT(ctx);
> + CTX_LOCK;
> + int rc = event_check_unlocked(gc, event_r, typemask, pred, pred_user);
> + CTX_UNLOCK;
> + GC_FREE;
> + return rc;
> +}
> +
> +int libxl_event_wait(libxl_ctx *ctx, libxl_event **event_r,
> + unsigned long typemask,
> + libxl_event_predicate *pred, void *pred_user) {
> + int rc;
> + struct timeval now;
> +
> + GC_INIT(ctx);
> + CTX_LOCK;
> +
> + for (;;) {
> + rc = event_check_unlocked(gc, event_r, typemask, pred, pred_user);
> + if (rc != ERROR_NOT_READY) goto out;
> +
> + rc = libxl__gettimeofday(gc, &now);
> + if (rc) goto out;
> +
> + int timeout;
> +
> + for (;;) {
> + int nfds = CTX->fd_polls_allocd;
> + timeout = -1;
> + rc = beforepoll_unlocked(gc, &nfds, CTX->fd_polls, &timeout,
> now);
> + if (!rc) break;
> + if (rc != ERROR_BUFFERFULL) goto out;
> +
> + struct pollfd *newarray =
> + (nfds > INT_MAX / sizeof(struct pollfd) / 2) ? 0 :
I think I've seen this construct before. I think it's worth a #define.
Hrm, was the other one a different struct in the sizeof? *scrobbles
around*. Yes, it was libxl__ev_fd* in that case and we were setting up
the before polled array.
However aren't these two arrays related (one is a shadow of the other)?
IOW isn't the real limit the minimum of the two cases?
> + realloc(CTX->fd_polls, sizeof(*newarray) * nfds);
> +
> + if (!newarray) { rc = ERROR_NOMEM; goto out; }
> +
> + CTX->fd_polls = newarray;
> + CTX->fd_polls_allocd = nfds;
> + }
> +
> + rc = poll(CTX->fd_polls, CTX->fd_polls_allocd, timeout);
> + if (rc < 0) {
> + if (errno == EINTR) continue;
> + LIBXL__LOG_ERRNOVAL(CTX, LIBXL__LOG_ERROR, errno, "poll failed");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + rc = libxl__gettimeofday(gc, &now);
> + if (rc) goto out;
> +
> + afterpoll_unlocked(gc, CTX->fd_polls_allocd, CTX->fd_polls, now);
> +
> + /* we unlock and free the gc each time we go through this loop,
> + * so that (a) we don't accumulate garbage and (b) any events
> + * which are to be dispatched by callback are actually delivered
> + * in a timely fashion.
> + */
> + CTX_UNLOCK;
> + libxl__free_all(gc);
> + CTX_LOCK;
> + }
> +
> + out:
> + CTX_UNLOCK;
> + GC_FREE;
> + return rc;
> +}
> +
> +/*
> * Local variables:
> * mode: C
> * c-basic-offset: 4
> diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
> index 25efbdf..bbe1ed0 100644
> --- a/tools/libxl/libxl_event.h
> +++ b/tools/libxl/libxl_event.h
[...]
> +/* Alternatively or additionally, the application may also use this: */
> +
> +typedef struct libxl_event_hooks {
> + uint64_t event_occurs_mask;
> + void (*event_occurs)(void *user, const libxl_event *event);
> + void (*disaster)(void *user, libxl_event_type type,
> + const char *msg, int errnoval);
> +} libxl_event_hooks;
> +
> +void libxl_event_register_callbacks(libxl_ctx *ctx,
> + const libxl_event_hooks *hooks, void
> *user);
> + /*
> + * Arranges that libxl will henceforth call event_occurs for any
> + * events whose type is set in event_occurs_mask, rather than
> + * queueing the event for retrieval by libxl_event_check/wait.
> + * Events whose bit is clear in mask are not affected.
Earlier on you said that applications can mix and match the style of
event retrieval they use. Is it the case that for any given event type
it will be delivered by exactly of the two mechanisms? IOW if an event
type is in event_occurs_mask it will never be delivered via
libxl_event_check/wait and vice versa?
> + *
> + * event becomes owned by the application and must be freed, either
> + * by event_occurs or later.
> + *
[...]
> + * 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.
Which is inevitably going to lead to disaster being called...
This only applies to every enabled event which is not delivered via the
callback mechanism, correct?
> 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.
> + */
> +
> +typedef struct libxl__evgen_domain_death libxl_evgen_domain_death;
> +int libxl_evenable_domain_death(libxl_ctx *ctx, uint32_t domid,
> + libxl_ev_user, libxl_evgen_domain_death
> **evgen_out);
> +void libxl_evdisable_domain_death(libxl_ctx *ctx, libxl_evgen_domain_death*);
> + /* Arranges for the generation of DOMAIN_SHUTDOWN and DOMAIN_DESTROY
> + * events. A domain which is destroyed before it shuts down
> + * may generate only a DESTROY event.
> + */
> +
> +typedef struct libxl__evgen_disk_eject libxl_evgen_disk_eject;
> +int libxl_evenable_disk_eject(libxl_ctx *ctx, uint32_t domid, const char
> *vdev,
> + libxl_ev_user, libxl_evgen_disk_eject **evgen_out);
> +void libxl_evdisable_disk_eject(libxl_ctx *ctx, libxl_evgen_disk_eject*);
> + /* Arranges for the generation of DISK_EJECT events. A copy of the
> + * string *vdev will be made for libxl's internal use, and a pointer
> + * to this (or some other) copy will be returned as the vdev
> + * member of event.u.
Should event.u.vdev therefore be const? (skipping forward to the IDL
addition it seems not to be)
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 88e7dbb..518a06d 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -154,11 +154,44 @@ typedef struct libxl__ev_watch_slot {
>
> libxl__ev_xswatch *libxl__watch_slot_contents(libxl__gc *gc, int slotnum);
>
> +
> +/*
> + * evgen structures, which are the state we use for generating
> + * events for the caller.
> + *
> + * In general in each case there's an internal and an external
> + * version of the _evdisable_FOO function; the internal one is
> + * used during cleanup.
> + */
> +
> +struct libxl__evgen_domain_death {
> + uint32_t domid;
> + unsigned shutdown_reported:1, death_reported:1;
> + LIBXL_TAILQ_ENTRY(libxl_evgen_domain_death) entry;
> + /* on list .death_reported ? CTX->death_list : CTX->death_reported */
Isn't this the other way round?
> + libxl_ev_user user;
> +};
> +_hidden void
> +libxl__evdisable_domain_death(libxl__gc*, libxl_evgen_domain_death*);
> +
[...]
> @@ -250,7 +295,9 @@ static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
> */
> /* register @ptr in @gc for free on exit from outermost libxl callframe. */
> _hidden int libxl__ptr_add(libxl__gc *gc, void *ptr);
> -/* if this is the outermost libxl callframe then free all pointers in @gc */
> +/* if this is the outermost libxl callframe then free all pointers in @gc
> + * and report all occurred events via callback, if applicable.
> + * May reenters the application so must not be called with ctx locked. */
reenter
> _hidden void libxl__free_all(libxl__gc *gc);
> /* allocate and zero @bytes. (similar to a gc'd malloc(3)+memzero()) */
> _hidden void *libxl__zalloc(libxl__gc *gc, int bytes);
[...]
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index d59d2cb..5a713f0 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -75,11 +75,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"),
> @@ -381,3 +376,33 @@ libxl_sched_credit = Struct("sched_credit", [
> ("weight", integer),
> ("cap", integer),
> ], dispose_fn=None)
> +
> +libxl_event_type = Enumeration("event_type", [
> + (1, "DOMAIN_SHUTDOWN"),
> + (2, "DOMAIN_DESTROY"),
> + (3, "DISK_EJECT"),
> + ])
> +
> +libxl_ev_user = Number("libxl_ev_user")
> +
> +libxl_ev_link = Builtin("ev_link", passby=PASS_BY_REFERENCE, private=True)
> +
> +libxl_event = Struct("event",[
> + ("link", libxl_ev_link,0,
The third member here is supposed to be a bool hence "False". I guess
python lets you get away with that.
(this aspect of the IDL sucks, it should allow for named parameters
instead of an opaque, variably size, tuple)
> + "for use by libxl; caller may use this once the event has been"
> + " returned by libxl_event_{check,wait}"),
> + ("domid", libxl_domid),
> + ("domuuid", libxl_uuid),
> + ("for_user", libxl_ev_user),
> + ("type", libxl_event_type),
> + ("u", KeyedUnion(None, libxl_event_type, "type",
> + [("domain_shutdown", Struct(None, [
> + ("shutdown_reason", uint8),
> + ])),
> + ("domain_destroy", Struct(None, [])),
> + ("disk_eject", Struct(None, [
> + ("vdev", string),
> + ("disk", libxl_device_disk),
> + ])),
> + ]))])
> +
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index f1e729c..e5738b7 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
[...]
> @@ -1439,10 +1465,11 @@ static int create_domain(struct domain_create
> *dom_info)
> const char *restore_file = dom_info->restore_file;
> int migrate_fd = dom_info->migrate_fd;
>
> - int fd, i;
> + int i;
> int need_daemon = daemonize;
> int ret, rc;
> - libxl_waiter *w1 = NULL, *w2 = NULL;
> + libxl_evgen_domain_death *deathw = NULL;
> + libxl_evgen_disk_eject **diskws = NULL; /* one per disk */
> void *config_data = 0;
> int config_len = 0;
> int restore_fd = -1;
> @@ -1647,14 +1674,14 @@ start:
> if (errno != EINTR) {
> perror("failed to wait for daemonizing child");
> ret = ERROR_FAIL;
> - goto error_out;
> + goto out;
> }
> }
> if (status) {
> libxl_report_child_exitstatus(ctx, XTL_ERROR,
> "daemonizing child", child1, status);
> ret = ERROR_FAIL;
> - goto error_out;
> + goto out;
> }
> ret = domid;
> goto out;
> @@ -1691,92 +1718,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);
> + 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;
> + }
Are all disks ejectable? I think only emulated CDROM devices are and
emulated disks and all PV devices are not. Should libxl provide a
predicate?
Of course it is harmless to wait for an eject on a device which can't...
> + while (1) {
> + libxl_event *event;
> + ret = domain_wait_event(&event);
> + if (ret) goto out;
> +
> + switch (event->type) {
> +
> + case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
> + LOG("Domain %d has shut down, reason code %d 0x%x", domid,
> + event->u.domain_shutdown.shutdown_reason,
> + event->u.domain_shutdown.shutdown_reason);
> + switch (handle_domain_death(ctx, domid, event, &d_config)) {
> + case 2:
> + if (!preserve_domain(ctx, domid, event, &d_config)) {
> + /* If we fail then exit leaving the old domain in place.
> */
> + ret = -1;
> + goto out;
> }
>
[...]
> + /* Otherwise fall through and restart. */
> + case 1:
> + libxl_event_free(ctx, event);
> + libxl_evdisable_domain_death(ctx, deathw);
> + deathw = NULL;
> + for (i = 0; i < d_config.num_disks; i++) {
> + libxl_evdisable_disk_eject(ctx, diskws[i]);
> + diskws[i] = NULL;
> }
[...]
> + /* discard any other events which may have been generated */
> + while (!(ret = libxl_event_check(ctx, &event,
> + LIBXL_EVENTMASK_ALL, 0,0)))
> {
> + libxl_event_free(ctx, event);
> }
[...]
> + if (ret != ERROR_NOT_READY) {
> + LOG("warning, libxl_event_check (cleanup) failed
> (rc=%d)",
> + ret);
> + }
This sees likely to be a commonly wanted pattern. Perhaps libxl could
provide a helper (or several) to disable all events and drain the queue?
[...]
> + /*
> + * XXX FIXME: If this sleep is not there then domain
> + * re-creation fails sometimes.
> + */
> + LOG("Done. Rebooting now");
> + sleep(2);
The optimistic part of me wonders if this is still the case...
> + goto start;
> +
> + case 0:
> + LOG("Done. Exiting now");
> + ret = 0;
> + goto out;
> +
> + default:
> + abort();
> + }
> +
> + case LIBXL_EVENT_TYPE_DOMAIN_DESTROY:
> + LOG("Domain %d has been destroyed.", domid);
> + ret = 0;
> + goto out;
> +
> + case LIBXL_EVENT_TYPE_DISK_EJECT:
> + /* XXX what is this for? */
> + libxl_cdrom_insert(ctx, domid, &event->u.disk_eject.disk);
Assuming &event->u.disk_eject.disk is an empty variant of the ejected
disk (like libxl_event_get_disk_eject_info used to return) then I think
this is "inserting" and empty device, i.e. ejecting it.
It might be more obvious to have libxl_cdrom_eject which takes the
"full" version of the disk and does the obvious thing (modifying the
disk to be empty)?
> + break;
> +
> + default:;
> + char *evstr = libxl_event_to_json(ctx, event);
> + LOG("warning, got unexpected event type %d, event=%s",
> + event->type, evstr);
> + free(evstr);
> }
[...]
> +
> + libxl_event_free(ctx, event);
> }
>
> error_out:
> @@ -2259,43 +2300,46 @@ static void destroy_domain(const char *p)
> static void shutdown_domain(const char *p, int wait)
> {
> int rc;
> + libxl_event *event;
>
> find_domain(p);
> rc=libxl_domain_shutdown(ctx, domid, 0);
> if (rc) { fprintf(stderr,"shutdown failed (rc=%d)\n",rc);exit(-1); }
>
> if (wait) {
[...]
> + libxl_evgen_domain_death *deathw;
>
[...]
> + rc = libxl_evenable_domain_death(ctx, domid, 0, &deathw);
> + if (rc) {
> + fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",rc);
> + exit(-1);
> + }
>
[...]
> + for (;;) {
> + rc = domain_wait_event(&event);
> + if (rc) exit(-1);
>
[...]
> + switch (event->type) {
>
[...]
> + case LIBXL_EVENT_TYPE_DOMAIN_DESTROY:
> + LOG("Domain %d has been destroyed", domid);
> + goto done;
>
[...]
> + case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
> + LOG("Domain %d has been shut down, reason code %d %x", domid,
> + event->u.domain_shutdown.shutdown_reason,
> + event->u.domain_shutdown.shutdown_reason);
> + goto done;
>
[...]
> + default:
> + LOG("Unexpected event type %d", event->type);
> + break;
> }
[...]
> + libxl_event_free(ctx, event);
> }
[...]
> + done:
> + libxl_event_free(ctx, event);
> + libxl_evdisable_domain_death(ctx, deathw);
> }
> }
>
> --
> 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |