(no subject)

(no subject)

[Ian Jackson]

This series removes the usleeps and waiting loops in libxl_dom.c and
replaces them with event-callback code.

Firstly, some documentation of things I had to reverse-engineer:
 01/23 xen: Document XEN_DOMCTL_subscribe
 02/23 xen: Document that EVTCHNOP_bind_interdomain signals
 03/23 docs: Document event-channel-based suspend protocol
 04/23 libxc: Document xenctrl.h event channel calls
Arguably these might be 4.4 material (hence the CC to George).

We want some additional libxl event facilities:
 05/23 libxl: init: Provide a gc later in libxl_ctx_alloc
 06/23 libxl: init: libxl__poller_init and _get take gc
 07/23 libxl: events: const-correct *_inuse, *_isregistered
 08/23 libxl: events: Provide libxl__xswait_*
 09/23 libxl: events: Use libxl__xswait_* in spawn code
 10/23 libxl: events: Provide libxl__ev_evtchn*

We need to clean up some unfortunate code in libxc:
 11/23 libxc: suspend: Rename, improve xc_suspend_evtchn_init
 12/23 libxc: suspend: Fix suspend event channel locking

We do some shuffling around of the libxl suspend control flow:
 13/23 libxl: suspend: Async libxl__domain_suspend_callback
 14/23 libxl: suspend: Async domain_suspend_callback_common
 15/23 libxl: suspend: Reorg domain_suspend_callback_common
 16/23 libxl: suspend: New libxl__domain_pvcontrol_xspath
 17/23 libxl: suspend: New domain_suspend_pvcontrol_acked
No functional change in those five.  These changes are broken down
just to make the changes reviewable.

Finally, we can start to work on the event code, removing the bugs,
usleeps and loops one at a time:
 18/23 libxl: suspend: domain_suspend_callback_common xs errs
 19/23 libxl: suspend: Async xenstore pvcontrol wait
 20/23 libxl: suspend: Abolish usleeps in domain suspend wait
 21/23 libxl: suspend: Fix suspend wait corner cases
 22/23 libxl: suspend: Async evtchn wait
 23/23 libxl: suspend: Apply guest timeout in evtchn case

I have tested this with a Debian pvops kernel (xenstore pvcontrol
suspend signalling) and OpenSUSE 11 (event channel suspend
signalling).

Shriram: I haven't really touched the Remus-specific code here but
this series ought to be suitable for you to base things on, assuming
my co-maintainers like the general approach.

Thanks,
Ian.

[PATCH 01/23] xen: Document XEN_DOMCTL_subscribe

[Ian Jackson]

Arguably this domctl is misnamed.  But, for now, document its actual
behaviour (reverse-engineered from the code and found in the commit
message for 4539594d46f9) under its actual name.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/libxc/xenctrl.h       |    4 +++-
 xen/include/public/domctl.h |   16 ++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 6e58ebe..096a590 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1452,8 +1452,10 @@ int xc_version(xc_interface *xch, int cmd, void *arg);
 int xc_flask_op(xc_interface *xch, xen_flask_op_t *op);
 
 /*
- * Subscribe to state changes in a domain via evtchn.
+ * Subscribe to domain suspend via evtchn.
  * Returns -1 on failure, in which case errno will be set appropriately.
+ * Just calls XEN_DOMCTL_subscribe - see the caveats for that domctl
+ * (in its doc comment in domctl.h).
  */
 int xc_domain_subscribe_for_suspend(
     xc_interface *xch, domid_t domid, evtchn_port_t port);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 01a3652..91f01fa 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -619,6 +619,22 @@ typedef struct xen_domctl_cpuid xen_domctl_cpuid_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_cpuid_t);
 #endif
 
+/*
+ * Arranges that if the domain suspends (specifically, if it shuts
+ * down with code SHUTDOWN_suspend), this event channel will be
+ * notified.
+ *
+ * This is _instead of_ the usual notification to the global
+ * VIRQ_DOM_EXC.  (In most systems that pirq is owned by xenstored.)
+ *
+ * Only one subscription per domain is possible.  Last subscriber
+ * wins; others are silently displaced.
+ *
+ * NB that contrary to the rather general name, it only applies to
+ * domain shutdown with code suspend.  Shutdown for other reasons
+ * (including crash), and domain death, are notified to VIRQ_DOM_EXC
+ * regardless.
+ */
 /* XEN_DOMCTL_subscribe */
 struct xen_domctl_subscribe {
     uint32_t port; /* IN */
-- 
1.7.10.4

[PATCH 02/23] xen: Document that EVTCHNOP_bind_interdomain signals

[Ian Jackson]

EVTCHNOP_bind_interdomain signals the event channel.  Document this.

Also explain the usual use pattern.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/include/public/event_channel.h |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/xen/include/public/event_channel.h b/xen/include/public/event_channel.h
index b78ee32..49ac8cc 100644
--- a/xen/include/public/event_channel.h
+++ b/xen/include/public/event_channel.h
@@ -101,6 +101,17 @@ typedef struct evtchn_alloc_unbound evtchn_alloc_unbound_t;
  * a port that is unbound and marked as accepting bindings from the calling
  * domain. A fresh port is allocated in the calling domain and returned as
  * <local_port>.
+ *
+ * In case the peer domain has already tried to set our event channel
+ * pending, before it was bound, EVTCHNOP_bind_interdomain always sets
+ * the local event channel pending.
+ *
+ * The usual pattern of use, in the guest's upcall (or subsequent
+ * handler) is as follows: (Re-enable the event channel for subsequent
+ * signalling and then) check for the existence of whatever condition
+ * is being waited for by other means, and take whatever action is
+ * needed (if any).
+ *
  * NOTES:
  *  1. <remote_dom> may be DOMID_SELF, allowing loopback connections.
  */
-- 
1.7.10.4

[PATCH 03/23] docs: Document event-channel-based suspend protocol

[Ian Jackson]

Document the event channel protocol in xenstore-paths.markdown, in the
section for ~/device/suspend/event-channel.

Protocol reverse-engineered from commentary and commit messages of
  4539594d46f9  Add facility to get notification of domain suspend ...
  17636f47a474  Teach xc_save to use event-channel-based ...
and implementations in
  xc_save (current version)
  libxl (current version)
  linux-2.6.18-xen (mercurial 1241:2993033a77ca)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 docs/misc/xenstore-paths.markdown |   23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index a0fc003..70ab7f4 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -143,10 +143,29 @@ The size of the video RAM this domain is configured with.
 
 #### ~/device/suspend/event-channel = ""|EVTCHN [w]
 
-The domain's suspend event channel. The use of a suspend event channel
-is optional at the domain's discretion. The toolstack will create this
+The domain's suspend event channel. The toolstack will create this
 path with an empty value which the guest may choose to overwrite.
 
+If the guest overwrites this, it will be with the number of an unbound
+event channel port it has acquired.  The toolstack is expected to use
+an interdomain bind, and then, when it wishes to ask the guest to
+suspend, to signal the event channel.
+
+The guest does not need to explicitly acknowledge the request; indeed,
+there is no explicit signalling by the guest in the reverse direction.
+The guest, when it is ready, simply shuts down (`SCHEDOP_shutdown`)
+with reason code `SHUTDOWN_suspend`.  The toolstack is expected to use
+`XEN_DOMCTL_subscribe` to be alerted to guest state changes, and
+`XEN_SYSCTL_getdomaininfolist` to verify that the domain has
+suspended.
+
+Note that the use of this event channel suspend protocol is optional
+for both sides.  By writing a non-empty string to the node, the guest
+is advertising its support.  However, the toolstack is at liberty to
+use the xenstore-based protocol instead (see ~/control/shutdown,
+below) even if the guest has advertised support for the event channel
+protocol.
+
 #### ~/hvmloader/generation-id-address = ADDRESS [r,HVM,INTERNAL]
 
 The hexadecimal representation of the address of the domain's
-- 
1.7.10.4

[PATCH 04/23] libxc: Document xenctrl.h event channel calls

[Ian Jackson]

Provide semantic documentation for how the libxc calls relate to the
hypervisor interface, and how they are to be used.

Also document the bug (present at least in Linux 3.12) that setting
the evtchn fd to nonblocking doesn't in fact make xc_evtchn_pending
nonblocking, and describe the appropriate workaround.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xenctrl.h |   27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 096a590..13f816b 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1043,6 +1043,18 @@ int xc_evtchn_close(xc_evtchn *xce);
 
 /*
  * Return an fd that can be select()ed on.
+ *
+ * Note that due to bugs, setting this fd to non blocking may not
+ * work: you would hope that it would result in xc_evtchn_pending
+ * failing with EWOULDBLOCK if there are no events signaled, but in
+ * fact it may block.  (Bug is present in at least Linux 3.12, and
+ * perhaps on other platforms or later version.)
+ *
+ * To be safe, you must use poll() or select() before each call to
+ * xc_evtchn_pending.  If you have multiple threads (or processes)
+ * sharing a single xce handle this will not work, and there is no
+ * straightforward workaround.  Please design your program some other
+ * way.
  */
 int xc_evtchn_fd(xc_evtchn *xce);
 
@@ -1082,7 +1094,20 @@ int xc_evtchn_unbind(xc_evtchn *xce, evtchn_port_t port);
 
 /*
  * Return the next event channel to become pending, or -1 on failure, in which
- * case errno will be set appropriately.  
+ * case errno will be set appropriately.
+ *
+ * At the hypervisor level the event channel will have been masked,
+ * and then cleared, by the underlying machinery (evtchn kernel
+ * driver, or equivalent).  So if the event channel is signaled again
+ * after it is returned here, it will be queued up, and delivered
+ * again after you unmask it.  (See the documentation in the Xen
+ * public header event_channel.h.)
+ *
+ * On receiving the notification from xc_evtchn_pending, you should
+ * normally: check (by other means) what work needs doing; do the
+ * necessary work (if any); unmask the event channel with
+ * xc_evtchn_unmask (if you want to receive any further
+ * notifications).
  */
 evtchn_port_or_error_t
 xc_evtchn_pending(xc_evtchn *xce);
-- 
1.7.10.4

[PATCH 05/23] libxl: init: Provide a gc later in libxl_ctx_alloc

[Ian Jackson]

Provide libxl__gc *gc for the second half of libxl_ctx_alloc.
(For the first half of the function, gc is in scope but set to NULL.)

This makes it possible to make gc-requiring calls.  For example, it
makes error logging more convenient.

Make use of this by changing the logging calls to use the LOG*
convenience macros.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e8ad610..cfd1435 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -25,6 +25,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
                     unsigned flags, xentoollog_logger * lg)
 {
     libxl_ctx *ctx = NULL;
+    libxl__gc gc_buf, *gc = NULL;
     int rc;
 
     if (version != LIBXL_VERSION) { rc = ERROR_VERSION; goto out; }
@@ -79,6 +80,9 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     }
 
     /* Now ctx is safe for ctx_free; failures simply set rc and "goto out" */
+    LIBXL_INIT_GC(gc_buf,ctx);
+    gc = &gc_buf;
+    /* Now gc is useable */
 
     rc = libxl__atfork_init(ctx);
     if (rc) goto out;
@@ -88,8 +92,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
 
     ctx->xch = xc_interface_open(lg,lg,0);
     if (!ctx->xch) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
-                        "cannot open libxc handle");
+        LOGEV(ERROR, errno, "cannot open libxc handle");
         rc = ERROR_FAIL; goto out;
     }
 
@@ -97,8 +100,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     if (!ctx->xsh)
         ctx->xsh = xs_domain_open();
     if (!ctx->xsh) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
-                        "cannot connect to xenstore");
+        LOGEV(ERROR, errno, "cannot connect to xenstore");
         rc = ERROR_FAIL; goto out;
     }
 
@@ -106,6 +108,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     return 0;
 
  out:
+    if (gc) libxl__free_all(gc);
     libxl_ctx_free(ctx);
     *pctx = NULL;
     return rc;
-- 
1.7.10.4

[PATCH 06/23] libxl: init: libxl__poller_init and _get take gc

[Ian Jackson]

Change libxl__poller_init and libxl__poller__get to take a libxl__gc*
rather than a libxl_ctx*.  The gc is not used for memory allocation
but simply to provide the standard local variable "gc" expected by the
convenience macros.  Doing this makes the error logging more
convenient.

Hence, convert the logging calls to use the LOG* convenience macros.

And consequently, change the call sites, and the function bodies to
use CTX rather than ctx.

Also convert a call to malloc() (with error check) in
libxl__poller_get, to libxl__zalloc (no error check needed).

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c          |    2 +-
 tools/libxl/libxl_event.c    |   25 ++++++++++---------------
 tools/libxl/libxl_internal.h |    4 ++--
 3 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index cfd1435..85b56a9 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -87,7 +87,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     rc = libxl__atfork_init(ctx);
     if (rc) goto out;
 
-    rc = libxl__poller_init(ctx, &ctx->poller_app);
+    rc = libxl__poller_init(gc, &ctx->poller_app);
     if (rc) goto out;
 
     ctx->xch = xc_interface_open(lg,lg,0);
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index bdef7ac..9c4fe1c 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1274,7 +1274,7 @@ int libxl_event_check(libxl_ctx *ctx, libxl_event **event_r,
  * Manipulation of pollers
  */
 
-int libxl__poller_init(libxl_ctx *ctx, libxl__poller *p)
+int libxl__poller_init(libxl__gc *gc, libxl__poller *p)
 {
     int r, rc;
     p->fd_polls = 0;
@@ -1282,15 +1282,15 @@ int libxl__poller_init(libxl_ctx *ctx, libxl__poller *p)
 
     r = pipe(p->wakeup_pipe);
     if (r) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot create poller pipe");
+        LOGE(ERROR, "cannot create poller pipe");
         rc = ERROR_FAIL;
         goto out;
     }
 
-    rc = libxl_fd_set_nonblock(ctx, p->wakeup_pipe[0], 1);
+    rc = libxl_fd_set_nonblock(CTX, p->wakeup_pipe[0], 1);
     if (rc) goto out;
 
-    rc = libxl_fd_set_nonblock(ctx, p->wakeup_pipe[1], 1);
+    rc = libxl_fd_set_nonblock(CTX, p->wakeup_pipe[1], 1);
     if (rc) goto out;
 
     return 0;
@@ -1308,25 +1308,20 @@ void libxl__poller_dispose(libxl__poller *p)
     free(p->fd_rindices);
 }
 
-libxl__poller *libxl__poller_get(libxl_ctx *ctx)
+libxl__poller *libxl__poller_get(libxl__gc *gc)
 {
     /* must be called with ctx locked */
     int rc;
 
-    libxl__poller *p = LIBXL_LIST_FIRST(&ctx->pollers_idle);
+    libxl__poller *p = LIBXL_LIST_FIRST(&CTX->pollers_idle);
     if (p) {
         LIBXL_LIST_REMOVE(p, entry);
         return p;
     }
 
-    p = malloc(sizeof(*p));
-    if (!p) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot allocate poller");
-        return 0;
-    }
-    memset(p, 0, sizeof(*p));
+    p = libxl__zalloc(NOGC, sizeof(*p));
 
-    rc = libxl__poller_init(ctx, p);
+    rc = libxl__poller_init(gc, p);
     if (rc) {
         free(p);
         return NULL;
@@ -1446,7 +1441,7 @@ int libxl_event_wait(libxl_ctx *ctx, libxl_event **event_r,
     EGC_INIT(ctx);
     CTX_LOCK;
 
-    poller = libxl__poller_get(ctx);
+    poller = libxl__poller_get(gc);
     if (!poller) { rc = ERROR_FAIL; goto out; }
 
     for (;;) {
@@ -1622,7 +1617,7 @@ libxl__ao *libxl__ao_create(libxl_ctx *ctx, uint32_t domid,
     if (how) {
         ao->how = *how;
     } else {
-        ao->poller = libxl__poller_get(ctx);
+        ao->poller = libxl__poller_get(&ao->gc);
         if (!ao->poller) goto out;
     }
     libxl__log(ctx,XTL_DEBUG,-1,file,line,func,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 1bd23ff..2712005 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -823,13 +823,13 @@ _hidden void libxl__event_disaster(libxl__egc*, const char *msg, int errnoval,
 
 /* Fills in, or disposes of, the resources held by, a poller whose
  * space the caller has allocated.  ctx must be locked. */
-_hidden int libxl__poller_init(libxl_ctx *ctx, libxl__poller *p);
+_hidden int libxl__poller_init(libxl__gc *gc, libxl__poller *p);
 _hidden void libxl__poller_dispose(libxl__poller *p);
 
 /* Obtain a fresh poller from malloc or the idle list, and put it
  * away again afterwards.  _get can fail, returning NULL.
  * ctx must be locked. */
-_hidden libxl__poller *libxl__poller_get(libxl_ctx *ctx);
+_hidden libxl__poller *libxl__poller_get(libxl__gc *gc);
 _hidden void libxl__poller_put(libxl_ctx*, libxl__poller *p /* may be NULL */);
 
 /* Notifies whoever is polling using p that they should wake up.
-- 
1.7.10.4

[PATCH 07/23] libxl: events: const-correct *_inuse, *_isregistered

[Ian Jackson]

The comments for libxl__ev_time_isregistered and the corresponding
watch function even say that these should be const.  Make it so.

Also fix libxl__ev_child_inuse and libxl__ev_spawn_inuse.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_internal.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2712005..51d6c6d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -716,7 +716,7 @@ _hidden int libxl__ev_fd_modify(libxl__gc*, libxl__ev_fd *ev,
 _hidden void libxl__ev_fd_deregister(libxl__gc*, libxl__ev_fd *ev);
 static inline void libxl__ev_fd_init(libxl__ev_fd *efd)
                     { efd->fd = -1; }
-static inline int libxl__ev_fd_isregistered(libxl__ev_fd *efd)
+static inline int libxl__ev_fd_isregistered(const libxl__ev_fd *efd)
                     { return efd->fd >= 0; }
 
 _hidden int libxl__ev_time_register_rel(libxl__gc*, libxl__ev_time *ev_out,
@@ -732,7 +732,7 @@ _hidden int libxl__ev_time_modify_abs(libxl__gc*, libxl__ev_time *ev,
 _hidden void libxl__ev_time_deregister(libxl__gc*, libxl__ev_time *ev);
 static inline void libxl__ev_time_init(libxl__ev_time *ev)
                 { ev->func = 0; }
-static inline int libxl__ev_time_isregistered(libxl__ev_time *ev)
+static inline int libxl__ev_time_isregistered(const libxl__ev_time *ev)
                 { return !!ev->func; }
 
 
@@ -772,7 +772,7 @@ _hidden pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child *childw_out,
                                  libxl__ev_child_callback *death);
 static inline void libxl__ev_child_init(libxl__ev_child *childw_out)
                 { childw_out->pid = -1; }
-static inline int libxl__ev_child_inuse(libxl__ev_child *childw_out)
+static inline int libxl__ev_child_inuse(const libxl__ev_child *childw_out)
                 { return childw_out->pid >= 0; }
 
 /* Useable (only) in the child to once more make the ctx useable for
@@ -1213,7 +1213,7 @@ struct libxl__spawn_state {
     libxl__ev_xswatch xswatch;
 };
 
-static inline int libxl__spawn_inuse(libxl__spawn_state *ss)
+static inline int libxl__spawn_inuse(const libxl__spawn_state *ss)
     { return libxl__ev_child_inuse(&ss->mid); }
 
 /*
-- 
1.7.10.4

[PATCH 08/23] libxl: events: Provide libxl__xswait_*

[Ian Jackson]

This is an ao utility for for conveniently doing a timed wait on
xenstore.  It handles setting up and cancelling the timeout, and also
conveniently reads the key for you.

No callers yet in this patch.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_aoutils.c  |   77 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |   52 ++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index b4eb6e5..477717b 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -16,6 +16,83 @@
 
 #include "libxl_internal.h"
 
+/*----- xswait -----*/
+
+static libxl__ev_xswatch_callback xswait_xswatch_callback;
+static libxl__ev_time_callback xswait_timeout_callback;
+static void xswait_report_error(libxl__egc*, libxl__xswait_state*, int rc);
+
+void libxl__xswait_init(libxl__xswait_state *xswa)
+{
+    libxl__ev_time_init(&xswa->time_ev);
+    libxl__ev_xswatch_init(&xswa->watch_ev);
+}
+
+void libxl__xswait_stop(libxl__gc *gc, libxl__xswait_state *xswa)
+{
+    libxl__ev_time_deregister(gc, &xswa->time_ev);
+    libxl__ev_xswatch_deregister(gc, &xswa->watch_ev);
+}
+
+bool libxl__xswait_inuse(const libxl__xswait_state *xswa)
+{
+    bool time_inuse = libxl__ev_time_isregistered(&xswa->time_ev);
+    bool watch_inuse = libxl__ev_xswatch_isregistered(&xswa->watch_ev);
+    assert(time_inuse == watch_inuse);
+    return time_inuse;
+}
+
+int libxl__xswait_start(libxl__gc *gc, libxl__xswait_state *xswa)
+{
+    int rc;
+
+    rc = libxl__ev_time_register_rel(gc, &xswa->time_ev,
+                                     xswait_timeout_callback, xswa->timeout_ms);
+    if (rc) goto err;
+
+    rc = libxl__ev_xswatch_register(gc, &xswa->watch_ev,
+                                    xswait_xswatch_callback, xswa->path);
+    if (rc) goto err;
+
+    return 0;
+
+ err:
+    libxl__xswait_stop(gc, xswa);
+    return rc;
+}
+
+void xswait_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *xsw,
+                             const char *watch_path, const char *event_path)
+{
+    EGC_GC;
+    libxl__xswait_state *xswa = CONTAINER_OF(xsw, *xswa, watch_ev);
+    int rc;
+    const char *data;
+
+    rc = libxl__xs_read_checked(gc, XBT_NULL, xswa->path, &data);
+    if (rc) { xswait_report_error(egc, xswa, rc); return; }
+
+    xswa->callback(egc, xswa, 0, data);
+}
+
+void xswait_timeout_callback(libxl__egc *egc, libxl__ev_time *ev,
+                             const struct timeval *requested_abs)
+{
+    EGC_GC;
+    libxl__xswait_state *xswa = CONTAINER_OF(ev, *xswa, time_ev);
+    LOG(DEBUG, "%s: xswait timeout (path=%s)", xswa->what, xswa->path);
+    xswait_report_error(egc, xswa, ERROR_TIMEDOUT);
+}
+
+static void xswait_report_error(libxl__egc *egc, libxl__xswait_state *xswa,
+                                int rc)
+{
+    EGC_GC;
+    libxl__xswait_stop(gc, xswa);
+    xswa->callback(egc, xswa, rc, 0);
+}
+
+
 /*----- data copier -----*/
 
 void libxl__datacopier_init(libxl__datacopier_state *dc)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 51d6c6d..36de135 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1068,6 +1068,58 @@ _hidden int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
                                       libxl_device_pci *pcidev, int num);
 _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid);
 
+/*----- xswait: wait for a xenstore node to be suitable -----*/
+
+typedef struct libxl__xswait_state libxl__xswait_state;
+
+/*
+ * rc describes the circumstances of this callback:
+ *
+ * rc==0
+ *
+ *     The xenstore path (may have) changed.  It has been read for
+ *     you.  The result is in data (allocated from the ao gc).
+ *     data may be NULL, which means that the xenstore read gave
+ *     ENOENT.
+ *
+ *     If you are satisfied, you MUST call libxl__xswait_stop.
+ *     Otherwise, xswait will continue waiting and watching and
+ *     will call you back later.
+ *
+ * rc==ETIMEDOUT
+ *
+ *     The specified timeout was reached.
+ *     This has NOT been logged (except to the debug log).
+ *     xswait will not continue (but calling libxl__xswait_stop is OK).
+ *
+ * rc!=0
+ *
+ *     Some other error occurred.
+ *     This HAS been logged.
+ *     xswait will not continue (but calling libxl__xswait_stop is OK).
+ *
+ */
+typedef void libxl__xswait_callback(libxl__egc *egc,
+      libxl__xswait_state *xswa, int rc, const char *data);
+
+struct libxl__xswait_state {
+    /* caller must fill these in, and they must all remain valid */
+    libxl__ao *ao;
+    const char *what; /* for error msgs: noun phrase, what we're waiting for */
+    const char *path;
+    int timeout_ms; /* as for poll(2) */
+    libxl__xswait_callback *callback;
+    /* remaining fields are private to xswait */
+    libxl__ev_time time_ev;
+    libxl__ev_xswatch watch_ev;
+};
+
+void libxl__xswait_init(libxl__xswait_state*);
+void libxl__xswait_stop(libxl__gc*, libxl__xswait_state*); /*idempotent*/
+bool libxl__xswait_inuse(const libxl__xswait_state *ss);
+
+int libxl__xswait_start(libxl__gc*, libxl__xswait_state*);
+
 /*
  *----- spawn -----
  *
-- 
1.7.10.4

[PATCH 09/23] libxl: events: Use libxl__xswait_* in spawn code

[Ian Jackson]

Replace open-coded use of ev_time and ev_xswatch with xswait.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_exec.c     |   49 +++++++++++++++---------------------------
 tools/libxl/libxl_internal.h |    3 +--
 2 files changed, 18 insertions(+), 34 deletions(-)

diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index b6efa0f..4b012dc 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -257,10 +257,8 @@ err:
  */
 
 /* Event callbacks. */
-static void spawn_watch_event(libxl__egc *egc, libxl__ev_xswatch *xsw,
-                              const char *watch_path, const char *event_path);
-static void spawn_timeout(libxl__egc *egc, libxl__ev_time *ev,
-                          const struct timeval *requested_abs);
+static void spawn_watch_event(libxl__egc *egc, libxl__xswait_state *xswa,
+                              int rc, const char *xsdata);
 static void spawn_middle_death(libxl__egc *egc, libxl__ev_child *childw,
                                pid_t pid, int status);
 
@@ -274,8 +272,7 @@ static void spawn_fail(libxl__egc *egc, libxl__spawn_state *ss);
 void libxl__spawn_init(libxl__spawn_state *ss)
 {
     libxl__ev_child_init(&ss->mid);
-    libxl__ev_time_init(&ss->timeout);
-    libxl__ev_xswatch_init(&ss->xswatch);
+    libxl__xswait_init(&ss->xswait);
 }
 
 int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
@@ -288,12 +285,12 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
     libxl__spawn_init(ss);
     ss->failed = ss->detaching = 0;
 
-    rc = libxl__ev_time_register_rel(gc, &ss->timeout,
-                                     spawn_timeout, ss->timeout_ms);
-    if (rc) goto out_err;
-
-    rc = libxl__ev_xswatch_register(gc, &ss->xswatch,
-                                    spawn_watch_event, ss->xspath);
+    ss->xswait.ao = ao;
+    ss->xswait.what = GCSPRINTF("%s startup", ss->what);
+    ss->xswait.path = ss->xspath;
+    ss->xswait.timeout_ms = ss->timeout_ms;
+    ss->xswait.callback = spawn_watch_event;
+    rc = libxl__xswait_start(gc, &ss->xswait);
     if (rc) goto out_err;
 
     pid_t middle = libxl__ev_child_fork(gc, &ss->mid, spawn_middle_death);
@@ -350,8 +347,7 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
 static void spawn_cleanup(libxl__gc *gc, libxl__spawn_state *ss)
 {
     assert(!libxl__ev_child_inuse(&ss->mid));
-    libxl__ev_time_deregister(gc, &ss->timeout);
-    libxl__ev_xswatch_deregister(gc, &ss->xswatch);
+    libxl__xswait_stop(gc, &ss->xswait);
 }
 
 static void spawn_detach(libxl__gc *gc, libxl__spawn_state *ss)
@@ -362,8 +358,7 @@ static void spawn_detach(libxl__gc *gc, libxl__spawn_state *ss)
     int r;
 
     assert(libxl__ev_child_inuse(&ss->mid));
-    libxl__ev_time_deregister(gc, &ss->timeout);
-    libxl__ev_xswatch_deregister(gc, &ss->xswatch);
+    libxl__xswait_stop(gc, &ss->xswait);
 
     pid_t child = ss->mid.pid;
     r = kill(child, SIGKILL);
@@ -387,25 +382,15 @@ static void spawn_fail(libxl__egc *egc, libxl__spawn_state *ss)
     spawn_detach(gc, ss);
 }
 
-static void spawn_timeout(libxl__egc *egc, libxl__ev_time *ev,
-                          const struct timeval *requested_abs)
-{
-    /* Before event, was Attached. */
-    EGC_GC;
-    libxl__spawn_state *ss = CONTAINER_OF(ev, *ss, timeout);
-    LOG(ERROR, "%s: startup timed out", ss->what);
-    spawn_fail(egc, ss); /* must be last */
-}
-
-static void spawn_watch_event(libxl__egc *egc, libxl__ev_xswatch *xsw,
-                              const char *watch_path, const char *event_path)
+static void spawn_watch_event(libxl__egc *egc, libxl__xswait_state *xswa,
+                              int rc, const char *p)
 {
     /* On entry, is Attached. */
     EGC_GC;
-    libxl__spawn_state *ss = CONTAINER_OF(xsw, *ss, xswatch);
-    char *p = libxl__xs_read(gc, 0, ss->xspath);
-    if (!p && errno != ENOENT) {
-        LOG(ERROR, "%s: xenstore read of %s failed", ss->what, ss->xspath);
+    libxl__spawn_state *ss = CONTAINER_OF(xswa, *ss, xswait);
+    if (rc) {
+        if (rc == ERROR_TIMEDOUT)
+            LOG(ERROR, "%s: startup timed out", ss->what);
         spawn_fail(egc, ss); /* must be last */
         return;
     }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 36de135..5d2e651 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1261,8 +1261,7 @@ struct libxl__spawn_state {
     int detaching; /* we are in Detaching */
     int failed; /* might be true whenever we are not Idle */
     libxl__ev_child mid; /* always in use whenever we are not Idle */
-    libxl__ev_time timeout;
-    libxl__ev_xswatch xswatch;
+    libxl__xswait_state xswait;
 };
 
 static inline int libxl__spawn_inuse(const libxl__spawn_state *ss)
-- 
1.7.10.4

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

[Ian Jackson]

This also involves providing a gc for the latter part of
libxl_ctx_alloc.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c          |    9 +++
 tools/libxl/libxl_event.c    |  152 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |   48 +++++++++++++
 3 files changed, 209 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 85b56a9..51158aa 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -60,6 +60,10 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     LIBXL_SLIST_INIT(&ctx->watch_freeslots);
     libxl__ev_fd_init(&ctx->watch_efd);
 
+    ctx->xce = 0;
+    LIBXL_LIST_INIT(&ctx->evtchns_waiting);
+    libxl__ev_fd_init(&ctx->evtchn_efd);
+
     LIBXL_TAILQ_INIT(&ctx->death_list);
     libxl__ev_xswatch_init(&ctx->death_watch);
 
@@ -104,6 +108,8 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
         rc = ERROR_FAIL; goto out;
     }
 
+    rc = libxl__ctx_evtchn_init(gc);
+
     *pctx = ctx;
     return 0;
 
@@ -147,16 +153,19 @@ int libxl_ctx_free(libxl_ctx *ctx)
     for (i = 0; i < ctx->watch_nslots; i++)
         assert(!libxl__watch_slot_contents(gc, i));
     libxl__ev_fd_deregister(gc, &ctx->watch_efd);
+    libxl__ev_fd_deregister(gc, &ctx->evtchn_efd);
     libxl__ev_fd_deregister(gc, &ctx->sigchld_selfpipe_efd);
 
     /* Now there should be no more events requested from the application: */
 
     assert(LIBXL_LIST_EMPTY(&ctx->efds));
     assert(LIBXL_TAILQ_EMPTY(&ctx->etimes));
+    assert(LIBXL_LIST_EMPTY(&ctx->evtchns_waiting));
 
     if (ctx->xch) xc_interface_close(ctx->xch);
     libxl_version_info_dispose(&ctx->version_info);
     if (ctx->xsh) xs_daemon_close(ctx->xsh);
+    if (ctx->xce) xc_evtchn_close(ctx->xce);
 
     libxl__poller_dispose(&ctx->poller_app);
     assert(LIBXL_LIST_EMPTY(&ctx->pollers_event));
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 9c4fe1c..838b620 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -614,6 +614,158 @@ void libxl__ev_xswatch_deregister(libxl__gc *gc, libxl__ev_xswatch *w)
 }
 
 /*
+ * evtchn
+ */
+
+static int evtchn_revents_check(libxl__egc *egc, int revents)
+{
+    EGC_GC;
+
+    if (revents & ~POLLIN) {
+        LOG(ERROR, "unexpected poll event on event channel fd: %x", revents);
+        LIBXL__EVENT_DISASTER(egc,
+                   "unexpected poll event on event channel fd", 0, 0);
+        libxl__ev_fd_deregister(gc, &CTX->evtchn_efd);
+        return ERROR_FAIL;
+    }
+
+    assert(revents & POLLIN);
+
+    return 0;
+}
+
+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;
+    struct pollfd recheck;
+
+    rc = evtchn_revents_check(egc, revents);
+    if (rc) return;
+
+    for (;;) {
+        /* Check the fd again.  The incoming revent may no longer be
+         * true, because the libxl ctx lock has not necessarily been
+         * held continuously since someone noticed the fd.  Normally
+         * this wouldn't be a problem but evtchn devices don't always
+         * honour O_NONBLOCK (see xenctrl.h). */
+
+        recheck.fd = fd;
+        recheck.events = POLLIN;
+        recheck.revents = 0;
+        r = poll(&recheck, 1, 0);
+        DBG("ev_evtchn recheck r=%d revents=%#x", r, recheck.revents);
+        if (r < 0) {
+            LIBXL__EVENT_DISASTER(egc,
+     "unexpected failure polling event channel fd for recheck",
+                                  errno, 0);
+            return;
+        }
+        if (r == 0)
+            break;
+        rc = evtchn_revents_check(egc, recheck.revents);
+        if (rc) return;
+
+        /* OK, that's that workaround done.  We can actually check for
+         * work for us to do: */
+
+        port = xc_evtchn_pending(CTX->xce);
+        if (port < 0) {
+            if (errno == EAGAIN)
+                break;
+            LIBXL__EVENT_DISASTER(egc,
+     "unexpected failure fetching occurring event port number from evtchn",
+                                  errno, 0);
+            return;
+        }
+
+        LIBXL_LIST_FOREACH(evev, &CTX->evtchns_waiting, entry)
+            if (port == evev->port)
+                goto found;
+        /* not found */
+        DBG("ev_evtchn port=%d no-one cared", port);
+        continue;
+
+    found:
+        DBG("ev_evtchn=%p port=%d signaled", evev, port);
+        evev->waiting = 0;
+        LIBXL_LIST_REMOVE(evev, entry);
+        evev->callback(egc, evev);
+    }
+}
+
+int libxl__ctx_evtchn_init(libxl__gc *gc) {
+    xc_evtchn *xce;
+    int rc, fd;
+
+    if (CTX->xce)
+        return 0;
+
+    xce = xc_evtchn_open(CTX->lg, 0);
+    if (!xce) {
+        LOGE(ERROR,"cannot open libxc evtchn handle");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    fd = xc_evtchn_fd(xce);
+    assert(fd >= 0);
+
+    rc = libxl_fd_set_nonblock(CTX, fd, 1);
+    if (rc) goto out;
+
+    rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd,
+                               evtchn_fd_callback, fd, POLLIN);
+    if (rc) goto out;
+
+    CTX->xce = xce;
+    return 0;
+
+ out:
+    xc_evtchn_close(xce);
+    return rc;
+}
+
+int libxl__ev_evtchn_wait(libxl__gc *gc, libxl__ev_evtchn *evev)
+{
+    int r, rc;
+
+    DBG("ev_evtchn=%p port=%d wait (was waiting=%d)",
+        evev, evev->port, evev->waiting);
+
+    if (evev->waiting)
+        return 0;
+
+    r = xc_evtchn_unmask(CTX->xce, evev->port);
+    if (r) {
+        LOGE(ERROR,"cannot unmask event channel %d",evev->port);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    evev->waiting = 1;
+    LIBXL_LIST_INSERT_HEAD(&CTX->evtchns_waiting, evev, entry);
+    return 0;
+
+ out:
+    return rc;
+}
+
+void libxl__ev_evtchn_cancel(libxl__gc *gc, libxl__ev_evtchn *evev)
+{
+    DBG("ev_evtchn=%p port=%d cancel (was waiting=%d)",
+        evev, evev->port, evev->waiting);
+
+    if (!evev->waiting)
+        return;
+
+    evev->waiting = 0;
+    LIBXL_LIST_REMOVE(evev, entry);
+}
+
+/*
  * waiting for device state
  */
 
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;
+    /* 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* */
+    LIBXL_LIST_HEAD(, libxl__ev_evtchn) evtchns_waiting;
+    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.
+ * 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).
+ */
+_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.
  *
-- 
1.7.10.4

[PATCH 11/23] libxc: suspend: Rename, improve xc_suspend_evtchn_init

[Ian Jackson]

xc_suspend_evtchn_init expects to eat the first event on the xce.  If
the xce is used for any other purpose then this can break.  Document
this fact and rename the function to xc_suspend_evtchn_init_exclusive.
(I haven't checked the call sites for improper shared use of the xce.)

Provide a corresponding xc_suspend_evtchn_init_sane which doesn't try
to eat an event, and instead leaves the caller the ability to
demultiplex.

Also document that xc_await_suspend needs exclusive use of the xce.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/libxc/xc_suspend.c                           |   21 ++++++++++++++++----
 tools/libxc/xenguest.h                             |   13 +++++++++++-
 tools/libxl/libxl_dom.c                            |    2 +-
 tools/misc/xen-hptool.c                            |    2 +-
 .../python/xen/lowlevel/checkpoint/libcheckpoint.c |    2 +-
 tools/xcutils/xc_save.c                            |    2 +-
 6 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
index 1ace411..7968a44 100644
--- a/tools/libxc/xc_suspend.c
+++ b/tools/libxc/xc_suspend.c
@@ -14,6 +14,8 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include <assert.h>
+
 #include "xc_private.h"
 #include "xenguest.h"
 
@@ -102,7 +104,7 @@ int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int
     return unlock_suspend_event(xch, domid);
 }
 
-int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int port)
+int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port)
 {
     int rc, suspend_evtchn = -1;
 
@@ -121,9 +123,6 @@ int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int por
         goto cleanup;
     }
 
-    /* event channel is pending immediately after binding */
-    xc_await_suspend(xch, xce, suspend_evtchn);
-
     return suspend_evtchn;
 
 cleanup:
@@ -132,3 +131,17 @@ cleanup:
 
     return -1;
 }
+
+int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port)
+{
+    int suspend_evtchn;
+
+    suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port);
+    if (suspend_evtchn < 0)
+        return suspend_evtchn;
+
+    /* event channel is pending immediately after binding */
+    xc_await_suspend(xch, xce, suspend_evtchn);
+
+    return suspend_evtchn;
+}
diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
index a0e30e1..ce5456c 100644
--- a/tools/libxc/xenguest.h
+++ b/tools/libxc/xenguest.h
@@ -256,10 +256,21 @@ int xc_hvm_build_target_mem(xc_interface *xch,
 
 int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn);
 
-int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int port);
+/**
+ * This function eats the initial notification.
+ * xce must not be used for anything else
+ */
+int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port);
 
+/* xce must not be used for anything else */
 int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn);
 
+/**
+ * The port will be signaled immediately after this call
+ * The caller should check the domain status and look for the next event
+ */
+int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port);
+
 int xc_get_bit_size(xc_interface *xch,
                     const char *image_name, const char *cmdline,
                     const char *features, int *type);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 55f74b2..4b42856 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1358,7 +1358,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
 
         if (port >= 0) {
             dss->suspend_eventchn =
-                xc_suspend_evtchn_init(CTX->xch, dss->xce, dss->domid, port);
+                xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce, dss->domid, port);
 
             if (dss->suspend_eventchn < 0)
                 LOG(WARN, "Suspend event channel initialization failed");
diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
index 24c3e95..db76f79 100644
--- a/tools/misc/xen-hptool.c
+++ b/tools/misc/xen-hptool.c
@@ -112,7 +112,7 @@ static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtc
         fprintf(stderr, "DOM%d: No suspend port, try live migration\n", domid);
         goto failed;
     }
-    suspend_evtchn = xc_suspend_evtchn_init(xch, xce, domid, port);
+    suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid, port);
     if (suspend_evtchn < 0)
     {
         fprintf(stderr, "Suspend evtchn initialization failed\n");
diff --git a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
index 01c0d47..817d272 100644
--- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
+++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
@@ -360,7 +360,7 @@ static int setup_suspend_evtchn(checkpoint_state* s)
     return -1;
   }
 
-  s->suspend_evtchn = xc_suspend_evtchn_init(s->xch, s->xce, s->domid, port);
+  s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce, s->domid, port);
   if (s->suspend_evtchn < 0) {
       s->errstr = "failed to bind suspend event channel";
       return -1;
diff --git a/tools/xcutils/xc_save.c b/tools/xcutils/xc_save.c
index e34bd2c..974f706 100644
--- a/tools/xcutils/xc_save.c
+++ b/tools/xcutils/xc_save.c
@@ -202,7 +202,7 @@ main(int argc, char **argv)
         else
         {
             si.suspend_evtchn =
-              xc_suspend_evtchn_init(si.xch, si.xce, si.domid, port);
+              xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid, port);
 
             if (si.suspend_evtchn < 0)
                 warnx("suspend event channel initialization failed, "
-- 
1.7.10.4

[PATCH 12/23] libxc: suspend: Fix suspend event channel locking

[Ian Jackson]

Use fcntl F_SETLK, rather than writing our pid into a "lock" file.
That way if we crash we don't leave the lockfile lying about.  Callers
now need to keep the fd for our lockfile.  (We don't use flock because
we don't want anyone who inherits this fd across fork to end up with a
handle onto the lock.)

While we are here:
 * Move the lockfile to /var/run/xen
 * De-duplicate the calculation of the pathname
 * Compute the buffer size for the pathname so that it will definitely
   not overrun (and use the computed value everywhere)
 * Fix various error handling bugs

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/libxc/xc_suspend.c                           |  159 +++++++++++++-------
 tools/libxc/xenguest.h                             |   16 +-
 tools/libxl/libxl_dom.c                            |    6 +-
 tools/libxl/libxl_internal.h                       |    1 +
 tools/misc/xen-hptool.c                            |   19 ++-
 tools/python/xen/lowlevel/checkpoint/checkpoint.h  |    2 +-
 .../python/xen/lowlevel/checkpoint/libcheckpoint.c |    7 +-
 tools/xcutils/xc_save.c                            |    8 +-
 8 files changed, 149 insertions(+), 69 deletions(-)

diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
index 7968a44..f0f70c1 100644
--- a/tools/libxc/xc_suspend.c
+++ b/tools/libxc/xc_suspend.c
@@ -15,66 +15,115 @@
  */
 
 #include <assert.h>
+#include <unistd.h>
+#include <fcntl.h>
 
 #include "xc_private.h"
 #include "xenguest.h"
 
-#define SUSPEND_LOCK_FILE "/var/lib/xen/suspend_evtchn"
-static int lock_suspend_event(xc_interface *xch, int domid)
+#define SUSPEND_LOCK_FILE "/var/run/xen/suspend-evtchn-%d.lock"
+
+/*
+ * locking
+ */
+
+#define ERR(x) do{                                                      \
+    ERROR("Can't " #x " lock file for suspend event channel %s: %s\n",  \
+          suspend_file, strerror(errno));                               \
+    goto err;                                                           \
+}while(0)
+
+#define SUSPEND_FILE_BUFLEN (sizeof(SUSPEND_LOCK_FILE) + 10)
+
+static void get_suspend_file(char buf[SUSPEND_FILE_BUFLEN], int domid)
 {
-    int fd, rc;
-    mode_t mask;
-    char buf[128];
-    char suspend_file[256];
-
-    snprintf(suspend_file, sizeof(suspend_file), "%s_%d_lock.d",
-	    SUSPEND_LOCK_FILE, domid);
-    mask = umask(022);
-    fd = open(suspend_file, O_CREAT | O_EXCL | O_RDWR, 0666);
-    if (fd < 0)
-    {
-        ERROR("Can't create lock file for suspend event channel %s\n",
-		suspend_file);
-        return -EINVAL;
+    snprintf(buf, sizeof(buf), SUSPEND_LOCK_FILE, domid);
+}
+
+static int lock_suspend_event(xc_interface *xch, int domid, int *lockfd)
+{
+    int fd = -1, r;
+    char suspend_file[SUSPEND_FILE_BUFLEN];
+    struct stat ours, theirs;
+    struct flock fl;
+
+    get_suspend_file(suspend_file, domid);
+
+    *lockfd = -1;
+
+    for (;;) {
+        if (fd >= 0)
+            close (fd);
+
+        fd = open(suspend_file, O_CREAT | O_RDWR, 0600);
+        if (fd < 0)
+            ERR("create");
+
+        r = fcntl(fd, F_SETFD, FD_CLOEXEC);
+        if (r)
+            ERR("fcntl F_SETFD FD_CLOEXEC");
+
+        memset(&fl, 0, sizeof(fl));
+        fl.l_type = F_WRLCK;
+        fl.l_whence = SEEK_SET;
+        fl.l_len = 1;
+        r = fcntl(fd, F_SETLK, &fl);
+        if (r)
+            ERR("fcntl F_SETLK");
+
+        r = fstat(fd, &ours);
+        if (r)
+            ERR("fstat");
+
+        r = stat(suspend_file, &theirs);
+        if (r) {
+            if (errno == ENOENT)
+                /* try again */
+                continue;
+            ERR("stat");
+        }
+
+        if (ours.st_ino != theirs.st_ino)
+            /* someone else must have removed it while we were locking it */
+            continue;
+
+        break;
     }
-    umask(mask);
-    snprintf(buf, sizeof(buf), "%10ld", (long)getpid());
 
-    rc = write_exact(fd, buf, strlen(buf));
-    close(fd);
+    *lockfd = fd;
+    return 0;
 
-    return rc;
+ err:
+    if (fd >= 0)
+        close(fd);
+
+    return -1;
 }
 
-static int unlock_suspend_event(xc_interface *xch, int domid)
+static int unlock_suspend_event(xc_interface *xch, int domid, int *lockfd)
 {
-    int fd, pid, n;
-    char buf[128];
-    char suspend_file[256];
+    int r;
+    char suspend_file[SUSPEND_FILE_BUFLEN];
 
-    snprintf(suspend_file, sizeof(suspend_file), "%s_%d_lock.d",
-	    SUSPEND_LOCK_FILE, domid);
-    fd = open(suspend_file, O_RDWR);
+    if (*lockfd < 0)
+        return 0;
 
-    if (fd < 0)
-        return -EINVAL;
+    get_suspend_file(suspend_file, domid);
 
-    n = read(fd, buf, 127);
+    r = unlink(suspend_file);
+    if (r)
+        ERR("unlink");
 
-    close(fd);
+    r = close(*lockfd);
+    *lockfd = -1;
+    if (r)
+        ERR("close");
 
-    if (n > 0)
-    {
-        sscanf(buf, "%d", &pid);
-        /* We are the owner, so we can simply delete the file */
-        if (pid == getpid())
-        {
-            unlink(suspend_file);
-            return 0;
-        }
-    }
+ err:
+    if (*lockfd >= 0)
+        close(*lockfd);
 
-    return -EPERM;
+    return -1;
 }
 
 int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn)
@@ -96,20 +145,26 @@ int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn)
     return 0;
 }
 
-int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn)
+/* Internal callers are allowed to call this with suspend_evtchn<0
+ * but *lockfd>0. */
+int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce,
+                              int domid, int suspend_evtchn, int *lockfd)
 {
     if (suspend_evtchn >= 0)
         xc_evtchn_unbind(xce, suspend_evtchn);
 
-    return unlock_suspend_event(xch, domid);
+    return unlock_suspend_event(xch, domid, lockfd);
 }
 
-int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port)
+int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce,
+                                int domid, int port, int *lockfd)
 {
     int rc, suspend_evtchn = -1;
 
-    if (lock_suspend_event(xch, domid))
-        return -EINVAL;
+    if (lock_suspend_event(xch, domid, lockfd)) {
+        errno = EINVAL;
+        goto cleanup;
+    }
 
     suspend_evtchn = xc_evtchn_bind_interdomain(xce, domid, port);
     if (suspend_evtchn < 0) {
@@ -126,17 +181,17 @@ int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, in
     return suspend_evtchn;
 
 cleanup:
-    if (suspend_evtchn != -1)
-        xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
+    xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn, lockfd);
 
     return -1;
 }
 
-int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port)
+int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce,
+                                     int domid, int port, int *lockfd)
 {
     int suspend_evtchn;
 
-    suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port);
+    suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port, lockfd);
     if (suspend_evtchn < 0)
         return suspend_evtchn;
 
diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
index ce5456c..1f216cd 100644
--- a/tools/libxc/xenguest.h
+++ b/tools/libxc/xenguest.h
@@ -254,13 +254,19 @@ int xc_hvm_build_target_mem(xc_interface *xch,
                             int target,
                             const char *image_name);
 
-int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn);
+/*
+ * Sets *lockfd to -1.
+ * Has deallocated everything even on error.
+ */
+int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn, int *lockfd);
 
 /**
  * This function eats the initial notification.
  * xce must not be used for anything else
+ * See xc_suspend_evtchn_init_sane re lockfd.
  */
-int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port);
+int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce,
+                                     int domid, int port, int *lockfd);
 
 /* xce must not be used for anything else */
 int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn);
@@ -268,8 +274,12 @@ int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn);
 /**
  * The port will be signaled immediately after this call
  * The caller should check the domain status and look for the next event
+ * On success, *lockfd will be set to >=0 and *lockfd must be preserved
+ * and fed to xc_suspend_evtchn_release.  (On error *lockfd is
+ * undefined and xc_suspend_evtchn_release is not allowed.)
  */
-int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port);
+int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce,
+                                int domid, int port, int *lockfd);
 
 int xc_get_bit_size(xc_interface *xch,
                     const char *image_name, const char *cmdline,
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 4b42856..48a4b8e 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1340,6 +1340,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
           | (dss->hvm ? XCFLAGS_HVM : 0);
 
     dss->suspend_eventchn = -1;
+    dss->guest_evtchn_lockfd = -1;
     dss->guest_responded = 0;
     dss->dm_savefile = libxl__device_model_savefile(gc, domid);
 
@@ -1358,7 +1359,8 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
 
         if (port >= 0) {
             dss->suspend_eventchn =
-                xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce, dss->domid, port);
+                xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce,
+                                  dss->domid, port, &dss->guest_evtchn_lockfd);
 
             if (dss->suspend_eventchn < 0)
                 LOG(WARN, "Suspend event channel initialization failed");
@@ -1522,7 +1524,7 @@ static void domain_suspend_done(libxl__egc *egc,
 
     if (dss->suspend_eventchn > 0)
         xc_suspend_evtchn_release(CTX->xch, dss->xce, domid,
-                                  dss->suspend_eventchn);
+                           dss->suspend_eventchn, &dss->guest_evtchn_lockfd);
     if (dss->xce != NULL)
         xc_evtchn_close(dss->xce);
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c519abc..01873c0 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2405,6 +2405,7 @@ struct libxl__domain_suspend_state {
     /* private */
     xc_evtchn *xce; /* event channel handle */
     int suspend_eventchn;
+    int guest_evtchn_lockfd;
     int hvm;
     int xcflags;
     int guest_responded;
diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
index db76f79..bd9d936 100644
--- a/tools/misc/xen-hptool.c
+++ b/tools/misc/xen-hptool.c
@@ -99,10 +99,13 @@ static int hp_mem_query_func(int argc, char *argv[])
 
 extern int xs_suspend_evtchn_port(int domid);
 
-static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtchn)
+static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid,
+                         int *evtchn, int *lockfd)
 {
     int port, rc, suspend_evtchn = -1;
 
+    *lockfd = -1;
+
     if (!evtchn)
         return -1;
 
@@ -112,7 +115,8 @@ static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtc
         fprintf(stderr, "DOM%d: No suspend port, try live migration\n", domid);
         goto failed;
     }
-    suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid, port);
+    suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid,
+                                                      port, lockfd);
     if (suspend_evtchn < 0)
     {
         fprintf(stderr, "Suspend evtchn initialization failed\n");
@@ -135,7 +139,8 @@ static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtc
 
 failed:
     if (suspend_evtchn != -1)
-        xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
+        xc_suspend_evtchn_release(xch, xce, domid,
+                                  suspend_evtchn, lockfd);
 
     return -1;
 }
@@ -193,7 +198,7 @@ static int hp_mem_offline_func(int argc, char *argv[])
                 }
                 else if (status & PG_OFFLINE_OWNED)
                 {
-                    int result, suspend_evtchn = -1;
+                    int result, suspend_evtchn = -1, suspend_lockfd = -1;
                     xc_evtchn *xce;
                     xce = xc_evtchn_open(NULL, 0);
 
@@ -205,7 +210,8 @@ static int hp_mem_offline_func(int argc, char *argv[])
                     }
 
                     domid = status >> PG_OFFLINE_OWNER_SHIFT;
-                    if (suspend_guest(xch, xce, domid, &suspend_evtchn))
+                    if (suspend_guest(xch, xce, domid,
+                                      &suspend_evtchn, &suspend_lockfd))
                     {
                         fprintf(stderr, "Failed to suspend guest %d for"
                                 " mfn %lx\n", domid, mfn);
@@ -231,7 +237,8 @@ static int hp_mem_offline_func(int argc, char *argv[])
                                 mfn, domid);
                     }
                     xc_domain_resume(xch, domid, 1);
-                    xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
+                    xc_suspend_evtchn_release(xch, xce, domid,
+                                              suspend_evtchn, &suspend_lockfd);
                     xc_evtchn_close(xce);
                 }
                 break;
diff --git a/tools/python/xen/lowlevel/checkpoint/checkpoint.h b/tools/python/xen/lowlevel/checkpoint/checkpoint.h
index 187d9d7..2414956 100644
--- a/tools/python/xen/lowlevel/checkpoint/checkpoint.h
+++ b/tools/python/xen/lowlevel/checkpoint/checkpoint.h
@@ -27,7 +27,7 @@ typedef struct {
     checkpoint_domtype domtype;
     int fd;
 
-    int suspend_evtchn;
+    int suspend_evtchn, suspend_lockfd;
 
     char* errstr;
 
diff --git a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
index 817d272..74ca062 100644
--- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
+++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
@@ -57,6 +57,7 @@ void checkpoint_init(checkpoint_state* s)
     s->fd = -1;
 
     s->suspend_evtchn = -1;
+    s->suspend_lockfd = -1;
 
     s->errstr = NULL;
 
@@ -360,7 +361,8 @@ static int setup_suspend_evtchn(checkpoint_state* s)
     return -1;
   }
 
-  s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce, s->domid, port);
+  s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce,
+                                    s->domid, port, &s->suspend_lockfd);
   if (s->suspend_evtchn < 0) {
       s->errstr = "failed to bind suspend event channel";
       return -1;
@@ -377,7 +379,8 @@ static void release_suspend_evtchn(checkpoint_state *s)
 {
   /* TODO: teach xen to clean up if port is unbound */
   if (s->xce != NULL && s->suspend_evtchn >= 0) {
-    xc_suspend_evtchn_release(s->xch, s->xce, s->domid, s->suspend_evtchn);
+    xc_suspend_evtchn_release(s->xch, s->xce, s->domid,
+                              s->suspend_evtchn, &s->suspend_lockfd);
     s->suspend_evtchn = -1;
   }
 }
diff --git a/tools/xcutils/xc_save.c b/tools/xcutils/xc_save.c
index 974f706..bf74e46 100644
--- a/tools/xcutils/xc_save.c
+++ b/tools/xcutils/xc_save.c
@@ -167,7 +167,7 @@ int
 main(int argc, char **argv)
 {
     unsigned int maxit, max_f, lflags;
-    int io_fd, ret, port;
+    int io_fd, ret, port, suspend_lockfd = -1;
     struct save_callbacks callbacks;
     xentoollog_level lvl;
     xentoollog_logger *l;
@@ -202,7 +202,8 @@ main(int argc, char **argv)
         else
         {
             si.suspend_evtchn =
-              xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid, port);
+              xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid,
+                                               port, &suspend_lockfd);
 
             if (si.suspend_evtchn < 0)
                 warnx("suspend event channel initialization failed, "
@@ -216,7 +217,8 @@ main(int argc, char **argv)
                          &callbacks, !!(si.flags & XCFLAGS_HVM), 0);
 
     if (si.suspend_evtchn > 0)
-	 xc_suspend_evtchn_release(si.xch, si.xce, si.domid, si.suspend_evtchn);
+	 xc_suspend_evtchn_release(si.xch, si.xce, si.domid,
+                                   si.suspend_evtchn, &suspend_lockfd);
 
     if (si.xce > 0)
         xc_evtchn_close(si.xce);
-- 
1.7.10.4

[PATCH 13/23] libxl: suspend: Async libxl__domain_suspend_callback

[Ian Jackson]

Mark the suspend callback libxl__domain_suspend_callback as
asynchronous in the helper stub generator (libxl_save_msgs_gen.pl).
We are going to want to provide an asynchronous version of this
function to get rid of the usleeps and waiting loops in the suspend
code.

libxl__domain_suspend_common_callback, the common synchronous core,
which used to be provided directly as the callback function for the
helper machinery, becomes libxl__domain_suspend_callback_common.  It
can now take a typesafe parameter.

For now, provide two very similar asynchronous wrappers for it.  Each
is a simple function which contains only boilerplate, calls the common
synchronous core, and returns the asynchronous response.

Essentially, we have just moved (in the case of suspend callbacks) the
call site of libxl__srm_callout_callback_complete.  It was in the
switch statement in the autogenerated _libxl_save_msgs_callout.c, and
is now in the handwritten libxl_dom.c.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
Cc: Ian Campbell <ian.campbell@citrix.com>

---
v2: Commit message mentions usleeps, not Remus, as motivation.
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_dom.c            |   25 +++++++++++++++++++------
 tools/libxl/libxl_internal.h       |    2 +-
 tools/libxl/libxl_save_msgs_gen.pl |    2 +-
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 48a4b8e..1c0f04f 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1020,10 +1020,8 @@ int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid)
     return 0;
 }
 
-int libxl__domain_suspend_common_callback(void *user)
+int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
 {
-    libxl__save_helper_state *shs = user;
-    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
     STATE_AO_GC(dss->ao);
     unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
     int ret;
@@ -1242,12 +1240,27 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
     return 0;
 }
 
+static void libxl__domain_suspend_callback(void *data)
+{
+    libxl__save_helper_state *shs = data;
+    libxl__egc *egc = shs->egc;
+    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
+
+    int ok = libxl__domain_suspend_callback_common(dss);
+    libxl__xc_domain_saverestore_async_callback_done(egc, shs, ok);
+}
+
 /*----- remus callbacks -----*/
 
-static int libxl__remus_domain_suspend_callback(void *data)
+static void libxl__remus_domain_suspend_callback(void *data)
 {
+    libxl__save_helper_state *shs = data;
+    libxl__egc *egc = shs->egc;
+    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
+
     /* REMUS TODO: Issue disk and network checkpoint reqs. */
-    return libxl__domain_suspend_common_callback(data);
+    int ok = libxl__domain_suspend_callback_common(dss);
+    libxl__xc_domain_saverestore_async_callback_done(egc, shs, ok);
 }
 
 static int libxl__remus_domain_resume_callback(void *data)
@@ -1373,7 +1386,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
         callbacks->postcopy = libxl__remus_domain_resume_callback;
         callbacks->checkpoint = libxl__remus_domain_checkpoint_callback;
     } else
-        callbacks->suspend = libxl__domain_suspend_common_callback;
+        callbacks->suspend = libxl__domain_suspend_callback;
 
     callbacks->switch_qemu_logdirty = libxl__domain_suspend_common_switch_qemu_logdirty;
     dss->shs.callbacks.save.toolstack_save = libxl__toolstack_save;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 01873c0..9b9264e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2705,7 +2705,7 @@ _hidden void libxl__xc_domain_save_done(libxl__egc*, void *dss_void,
 void libxl__xc_domain_saverestore_async_callback_done(libxl__egc *egc,
                            libxl__save_helper_state *shs, int return_value);
 
-_hidden int libxl__domain_suspend_common_callback(void *data);
+_hidden int libxl__domain_suspend_callback_common(libxl__domain_suspend_state*);
 _hidden void libxl__domain_suspend_common_switch_qemu_logdirty
                                (int domid, unsigned int enable, void *data);
 _hidden int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
diff --git a/tools/libxl/libxl_save_msgs_gen.pl b/tools/libxl/libxl_save_msgs_gen.pl
index ee126c7..3c6bd57 100755
--- a/tools/libxl/libxl_save_msgs_gen.pl
+++ b/tools/libxl/libxl_save_msgs_gen.pl
@@ -23,7 +23,7 @@ our @msgs = (
                                                  STRING doing_what),
                                                 'unsigned long', 'done',
                                                 'unsigned long', 'total'] ],
-    [  3, 'scxW',   "suspend", [] ],         
+    [  3, 'scxA',   "suspend", [] ],         
     [  4, 'scxW',   "postcopy", [] ],        
     [  5, 'scxA',   "checkpoint", [] ],      
     [  6, 'scxA',   "switch_qemu_logdirty",  [qw(int domid
-- 
1.7.10.4

[PATCH 14/23] libxl: suspend: Async domain_suspend_callback_common

[Ian Jackson]

Make domain_suspend_callback_common do its work and then call
dss->callback_common_done, rather than simply returning its answer.

This is preparatory to abolishing the usleeps in this function and
replacing them with use of the event machinery.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_dom.c      |   46 ++++++++++++++++++++++++++++++------------
 tools/libxl/libxl_internal.h |    4 +++-
 2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 1c0f04f..39f06da 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -755,6 +755,10 @@ int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
 
 static void domain_suspend_done(libxl__egc *egc,
                         libxl__domain_suspend_state *dss, int rc);
+static void domain_suspend_callback_common_done(libxl__egc *egc,
+                                libxl__domain_suspend_state *dss, int ok);
+static void remus_domain_suspend_callback_common_done(libxl__egc *egc,
+                                libxl__domain_suspend_state *dss, int ok);
 
 /*----- complicated callback, called by xc_domain_save -----*/
 
@@ -1020,7 +1024,9 @@ int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid)
     return 0;
 }
 
-int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
+/* calls dss->callback_common_done when done */
+static void domain_suspend_callback_common(libxl__egc *egc,
+                                           libxl__domain_suspend_state *dss)
 {
     STATE_AO_GC(dss->ao);
     unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
@@ -1043,12 +1049,12 @@ int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
         ret = xc_evtchn_notify(dss->xce, dss->suspend_eventchn);
         if (ret < 0) {
             LOG(ERROR, "xc_evtchn_notify failed ret=%d", ret);
-            return 0;
+            goto err;
         }
         ret = xc_await_suspend(CTX->xch, dss->xce, dss->suspend_eventchn);
         if (ret < 0) {
             LOG(ERROR, "xc_await_suspend failed ret=%d", ret);
-            return 0;
+            goto err;
         }
         dss->guest_responded = 1;
         goto guest_suspended;
@@ -1059,7 +1065,7 @@ int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
         ret = xc_domain_shutdown(CTX->xch, domid, SHUTDOWN_suspend);
         if (ret < 0) {
             LOGE(ERROR, "xc_domain_shutdown failed");
-            return 0;
+            goto err;
         }
         /* The guest does not (need to) respond to this sort of request. */
         dss->guest_responded = 1;
@@ -1113,7 +1119,7 @@ int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
          */
         if (!strcmp(state, "suspend")) {
             LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
-            return 0;
+            goto err;
         }
 
         LOG(DEBUG, "guest acknowledged suspend request");
@@ -1143,17 +1149,19 @@ int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
     }
 
     LOG(ERROR, "guest did not suspend");
-    return 0;
+ err:
+    dss->callback_common_done(egc, dss, 0);
+    return;
 
  guest_suspended:
     if (dss->hvm) {
         ret = libxl__domain_suspend_device_model(gc, dss);
         if (ret) {
             LOG(ERROR, "libxl__domain_suspend_device_model failed ret=%d", ret);
-            return 0;
+            goto err;
         }
     }
-    return 1;
+    dss->callback_common_done(egc, dss, 1);
 }
 
 static inline char *physmap_path(libxl__gc *gc, uint32_t domid,
@@ -1246,10 +1254,16 @@ static void libxl__domain_suspend_callback(void *data)
     libxl__egc *egc = shs->egc;
     libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
 
-    int ok = libxl__domain_suspend_callback_common(dss);
-    libxl__xc_domain_saverestore_async_callback_done(egc, shs, ok);
+    dss->callback_common_done = domain_suspend_callback_common_done;
+    domain_suspend_callback_common(egc, dss);
 }
 
+static void domain_suspend_callback_common_done(libxl__egc *egc,
+                                libxl__domain_suspend_state *dss, int ok)
+{
+    libxl__xc_domain_saverestore_async_callback_done(egc, &dss->shs, ok);
+}    
+
 /*----- remus callbacks -----*/
 
 static void libxl__remus_domain_suspend_callback(void *data)
@@ -1258,11 +1272,17 @@ static void libxl__remus_domain_suspend_callback(void *data)
     libxl__egc *egc = shs->egc;
     libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
 
-    /* REMUS TODO: Issue disk and network checkpoint reqs. */
-    int ok = libxl__domain_suspend_callback_common(dss);
-    libxl__xc_domain_saverestore_async_callback_done(egc, shs, ok);
+    dss->callback_common_done = remus_domain_suspend_callback_common_done;
+    domain_suspend_callback_common(egc, dss);
 }
 
+static void remus_domain_suspend_callback_common_done(libxl__egc *egc,
+                                libxl__domain_suspend_state *dss, int ok)
+{
+    /* REMUS TODO: Issue disk and network checkpoint reqs. */
+    libxl__xc_domain_saverestore_async_callback_done(egc, &dss->shs, ok);
+}    
+
 static int libxl__remus_domain_resume_callback(void *data)
 {
     libxl__save_helper_state *shs = data;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9b9264e..55293f1 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2413,6 +2413,8 @@ struct libxl__domain_suspend_state {
     int interval; /* checkpoint interval (for Remus) */
     libxl__save_helper_state shs;
     libxl__logdirty_switch logdirty;
+    void (*callback_common_done)(libxl__egc*,
+                                 struct libxl__domain_suspend_state*, int ok);
     /* private for libxl__domain_save_device_model */
     libxl__save_device_model_cb *save_dm_callback;
     libxl__datacopier_state save_dm_datacopier;
@@ -2705,7 +2707,7 @@ _hidden void libxl__xc_domain_save_done(libxl__egc*, void *dss_void,
 void libxl__xc_domain_saverestore_async_callback_done(libxl__egc *egc,
                            libxl__save_helper_state *shs, int return_value);
 
-_hidden int libxl__domain_suspend_callback_common(libxl__domain_suspend_state*);
+
 _hidden void libxl__domain_suspend_common_switch_qemu_logdirty
                                (int domid, unsigned int enable, void *data);
 _hidden int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
-- 
1.7.10.4

[PATCH 15/23] libxl: suspend: Reorg domain_suspend_callback_common

[Ian Jackson]

Make domain_suspend_callback_common more callback-oriented:

* Turn the functionality behind the goto labels "err" and
  "guest_suspended" into functions which can be called just before
  "return".

* Deindent the "issuing %s suspend request via XenBus control node"
  branch; it is going to be split up into various functions as the
  xenstore work becomes callback-based.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_dom.c |  158 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 103 insertions(+), 55 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 39f06da..59f6ce3 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1024,6 +1024,16 @@ int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid)
     return 0;
 }
 
+static void domain_suspend_common_wait_guest(libxl__egc *egc,
+                                             libxl__domain_suspend_state *dss);
+static void domain_suspend_common_guest_suspended(libxl__egc *egc,
+                                         libxl__domain_suspend_state *dss);
+static void domain_suspend_common_failed(libxl__egc *egc,
+                                         libxl__domain_suspend_state *dss);
+static void domain_suspend_common_done(libxl__egc *egc,
+                                       libxl__domain_suspend_state *dss,
+                                       bool ok);
+
 /* calls dss->callback_common_done when done */
 static void domain_suspend_callback_common(libxl__egc *egc,
                                            libxl__domain_suspend_state *dss)
@@ -1057,7 +1067,8 @@ static void domain_suspend_callback_common(libxl__egc *egc,
             goto err;
         }
         dss->guest_responded = 1;
-        goto guest_suspended;
+        domain_suspend_common_guest_suspended(egc, dss);
+        return;
     }
 
     if (dss->hvm && (!hvm_pvdrv || hvm_s_state)) {
@@ -1069,63 +1080,81 @@ static void domain_suspend_callback_common(libxl__egc *egc,
         }
         /* The guest does not (need to) respond to this sort of request. */
         dss->guest_responded = 1;
-    } else {
-        LOG(DEBUG, "issuing %s suspend request via XenBus control node",
-            dss->hvm ? "PVHVM" : "PV");
+        domain_suspend_common_wait_guest(egc, dss);
+        return;
+    }
 
-        libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "suspend");
+    LOG(DEBUG, "issuing %s suspend request via XenBus control node",
+        dss->hvm ? "PVHVM" : "PV");
 
-        LOG(DEBUG, "wait for the guest to acknowledge suspend request");
-        watchdog = 60;
-        while (!strcmp(state, "suspend") && watchdog > 0) {
-            usleep(100000);
+    libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "suspend");
 
-            state = libxl__domain_pvcontrol_read(gc, XBT_NULL, domid);
-            if (!state) state = "";
+    LOG(DEBUG, "wait for the guest to acknowledge suspend request");
+    watchdog = 60;
+    while (!strcmp(state, "suspend") && watchdog > 0) {
+        usleep(100000);
 
-            watchdog--;
-        }
+        state = libxl__domain_pvcontrol_read(gc, XBT_NULL, domid);
+        if (!state) state = "";
 
-        /*
-         * Guest appears to not be responding. Cancel the suspend
-         * request.
-         *
-         * We re-read the suspend node and clear it within a
-         * transaction in order to handle the case where we race
-         * against the guest catching up and acknowledging the request
-         * at the last minute.
-         */
-        if (!strcmp(state, "suspend")) {
-            LOG(ERROR, "guest didn't acknowledge suspend, cancelling request");
-        retry_transaction:
-            t = xs_transaction_start(CTX->xsh);
-
-            state = libxl__domain_pvcontrol_read(gc, t, domid);
-            if (!state) state = "";
-
-            if (!strcmp(state, "suspend"))
-                libxl__domain_pvcontrol_write(gc, t, domid, "");
-
-            if (!xs_transaction_end(CTX->xsh, t, 0))
-                if (errno == EAGAIN)
-                    goto retry_transaction;
+        watchdog--;
+    }
 
-        }
+    /*
+     * Guest appears to not be responding. Cancel the suspend
+     * request.
+     *
+     * We re-read the suspend node and clear it within a
+     * transaction in order to handle the case where we race
+     * against the guest catching up and acknowledging the request
+     * at the last minute.
+     */
+    if (!strcmp(state, "suspend")) {
+        LOG(ERROR, "guest didn't acknowledge suspend, cancelling request");
+    retry_transaction:
+        t = xs_transaction_start(CTX->xsh);
 
-        /*
-         * Final check for guest acknowledgement. The guest may have
-         * acknowledged while we were cancelling the request in which
-         * case we lost the race while cancelling and should continue.
-         */
-        if (!strcmp(state, "suspend")) {
-            LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
-            goto err;
-        }
+        state = libxl__domain_pvcontrol_read(gc, t, domid);
+        if (!state) state = "";
+
+        if (!strcmp(state, "suspend"))
+            libxl__domain_pvcontrol_write(gc, t, domid, "");
+
+        if (!xs_transaction_end(CTX->xsh, t, 0))
+            if (errno == EAGAIN)
+                goto retry_transaction;
 
-        LOG(DEBUG, "guest acknowledged suspend request");
-        dss->guest_responded = 1;
     }
 
+    /*
+     * Final check for guest acknowledgement. The guest may have
+     * acknowledged while we were cancelling the request in which
+     * case we lost the race while cancelling and should continue.
+     */
+    if (!strcmp(state, "suspend")) {
+        LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
+        goto err;
+    }
+
+    LOG(DEBUG, "guest acknowledged suspend request");
+    dss->guest_responded = 1;
+    domain_suspend_common_wait_guest(egc,dss);
+    return;
+
+ err:
+    domain_suspend_common_failed(egc, dss);
+}
+
+static void domain_suspend_common_wait_guest(libxl__egc *egc,
+                                             libxl__domain_suspend_state *dss)
+{
+    STATE_AO_GC(dss->ao);
+    int ret;
+    int watchdog;
+
+    /* Convenience aliases */
+    const uint32_t domid = dss->domid;
+
     LOG(DEBUG, "wait for the guest to suspend");
     watchdog = 60;
     while (watchdog > 0) {
@@ -1141,7 +1170,8 @@ static void domain_suspend_callback_common(libxl__egc *egc,
                 & XEN_DOMINF_shutdownmask;
             if (shutdown_reason == SHUTDOWN_suspend) {
                 LOG(DEBUG, "guest has suspended");
-                goto guest_suspended;
+                domain_suspend_common_guest_suspended(egc, dss);
+                return;
             }
         }
 
@@ -1149,19 +1179,37 @@ static void domain_suspend_callback_common(libxl__egc *egc,
     }
 
     LOG(ERROR, "guest did not suspend");
- err:
-    dss->callback_common_done(egc, dss, 0);
-    return;
+    domain_suspend_common_failed(egc, dss);
+}
+
+static void domain_suspend_common_guest_suspended(libxl__egc *egc,
+                                         libxl__domain_suspend_state *dss)
+{
+    STATE_AO_GC(dss->ao);
+    int ret;
 
- guest_suspended:
     if (dss->hvm) {
         ret = libxl__domain_suspend_device_model(gc, dss);
         if (ret) {
             LOG(ERROR, "libxl__domain_suspend_device_model failed ret=%d", ret);
-            goto err;
+            domain_suspend_common_failed(egc, dss);
+            return;
         }
     }
-    dss->callback_common_done(egc, dss, 1);
+    domain_suspend_common_done(egc, dss, 1);
+}
+
+static void domain_suspend_common_failed(libxl__egc *egc,
+                                         libxl__domain_suspend_state *dss)
+{
+    domain_suspend_common_done(egc, dss, 0);
+}
+
+static void domain_suspend_common_done(libxl__egc *egc,
+                                       libxl__domain_suspend_state *dss,
+                                       bool ok)
+{
+    dss->callback_common_done(egc, dss, ok);
 }
 
 static inline char *physmap_path(libxl__gc *gc, uint32_t domid,
-- 
1.7.10.4

[PATCH 16/23] libxl: suspend: New libxl__domain_pvcontrol_xspath

[Ian Jackson]

Factor out the pv control node xenstore path calculation into
libxl__domain_pvcontrol_xspath.

This xs path calculation was open coded in
libxl__domain_pvcontrol_read and _write.  This is undesirable because
it duplicates the code and because it makes the path inaccessible to
other parts of libxl (which are soon going to want it).

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c          |   21 +++++++++++----------
 tools/libxl/libxl_internal.h |    1 +
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 51158aa..8d70a45 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -898,17 +898,23 @@ int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid)
     return !!pvdriver;
 }
 
-char * libxl__domain_pvcontrol_read(libxl__gc *gc, xs_transaction_t t,
-                                    uint32_t domid)
+const char *libxl__domain_pvcontrol_xspath(libxl__gc *gc, uint32_t domid)
 {
-    const char *shutdown_path;
     const char *dom_path;
 
     dom_path = libxl__xs_get_dompath(gc, domid);
     if (!dom_path)
         return NULL;
 
-    shutdown_path = libxl__sprintf(gc, "%s/control/shutdown", dom_path);
+    return GCSPRINTF("%s/control/shutdown", dom_path);
+}
+
+char * libxl__domain_pvcontrol_read(libxl__gc *gc, xs_transaction_t t,
+                                    uint32_t domid)
+{
+    const char *shutdown_path;
+
+    shutdown_path = libxl__domain_pvcontrol_xspath(gc, domid);
     if (!shutdown_path)
         return NULL;
 
@@ -919,13 +925,8 @@ int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
                                   uint32_t domid, const char *cmd)
 {
     const char *shutdown_path;
-    const char *dom_path;
-
-    dom_path = libxl__xs_get_dompath(gc, domid);
-    if (!dom_path)
-        return ERROR_FAIL;
 
-    shutdown_path = libxl__sprintf(gc, "%s/control/shutdown", dom_path);
+    shutdown_path = libxl__domain_pvcontrol_xspath(gc, domid);
     if (!shutdown_path)
         return ERROR_FAIL;
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 55293f1..93e0d4e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -967,6 +967,7 @@ _hidden int libxl__domain_resume(libxl__gc *gc, uint32_t domid,
 /* returns 0 or 1, or a libxl error code */
 _hidden int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid);
 
+_hidden const char *libxl__domain_pvcontrol_xspath(libxl__gc*, uint32_t domid);
 _hidden char * libxl__domain_pvcontrol_read(libxl__gc *gc,
                                             xs_transaction_t t, uint32_t domid);
 _hidden int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
-- 
1.7.10.4

[PATCH 17/23] libxl: suspend: New domain_suspend_pvcontrol_acked

[Ian Jackson]

Factor out domain_suspend_pvcontrol_acked.

This replaces a bunch of open-coded strcmp()s and makes the code
clearer.  It also eliminates the need to check for state==NULL each
time it's read, because we can check for NULL once before the strcmp.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_dom.c |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 59f6ce3..a0b3a57 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1034,6 +1034,12 @@ static void domain_suspend_common_done(libxl__egc *egc,
                                        libxl__domain_suspend_state *dss,
                                        bool ok);
 
+static bool domain_suspend_pvcontrol_acked(const char *state) {
+    /* any value other than "suspend", including ENOENT, is OK */
+    if (!state) return 1;
+    return strcmp(state,"suspend");
+}
+
 /* calls dss->callback_common_done when done */
 static void domain_suspend_callback_common(libxl__egc *egc,
                                            libxl__domain_suspend_state *dss)
@@ -1091,11 +1097,10 @@ static void domain_suspend_callback_common(libxl__egc *egc,
 
     LOG(DEBUG, "wait for the guest to acknowledge suspend request");
     watchdog = 60;
-    while (!strcmp(state, "suspend") && watchdog > 0) {
+    while (!domain_suspend_pvcontrol_acked(state) && watchdog > 0) {
         usleep(100000);
 
         state = libxl__domain_pvcontrol_read(gc, XBT_NULL, domid);
-        if (!state) state = "";
 
         watchdog--;
     }
@@ -1109,21 +1114,19 @@ static void domain_suspend_callback_common(libxl__egc *egc,
      * against the guest catching up and acknowledging the request
      * at the last minute.
      */
-    if (!strcmp(state, "suspend")) {
+    if (!domain_suspend_pvcontrol_acked(state)) {
         LOG(ERROR, "guest didn't acknowledge suspend, cancelling request");
     retry_transaction:
         t = xs_transaction_start(CTX->xsh);
 
         state = libxl__domain_pvcontrol_read(gc, t, domid);
-        if (!state) state = "";
 
-        if (!strcmp(state, "suspend"))
+        if (!domain_suspend_pvcontrol_acked(state))
             libxl__domain_pvcontrol_write(gc, t, domid, "");
 
         if (!xs_transaction_end(CTX->xsh, t, 0))
             if (errno == EAGAIN)
                 goto retry_transaction;
-
     }
 
     /*
@@ -1131,7 +1134,7 @@ static void domain_suspend_callback_common(libxl__egc *egc,
      * acknowledged while we were cancelling the request in which
      * case we lost the race while cancelling and should continue.
      */
-    if (!strcmp(state, "suspend")) {
+    if (!domain_suspend_pvcontrol_acked(state)) {
         LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
         goto err;
     }
-- 
1.7.10.4

[PATCH 18/23] libxl: suspend: domain_suspend_callback_common xs errs

[Ian Jackson]

In domain_suspend_callback_common, use libxl__xs_transaction_start in
a loop, rather than xs_transaction_start and a goto label.

This will improve the error handling, but have no other semantic
effect.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_dom.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index a0b3a57..8aceba9 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1050,6 +1050,7 @@ static void domain_suspend_callback_common(libxl__egc *egc,
     char *state = "suspend";
     int watchdog;
     xs_transaction_t t;
+    int rc;
 
     /* Convenience aliases */
     const uint32_t domid = dss->domid;
@@ -1116,17 +1117,19 @@ static void domain_suspend_callback_common(libxl__egc *egc,
      */
     if (!domain_suspend_pvcontrol_acked(state)) {
         LOG(ERROR, "guest didn't acknowledge suspend, cancelling request");
-    retry_transaction:
-        t = xs_transaction_start(CTX->xsh);
+        for (;;) {
+            rc = libxl__xs_transaction_start(gc, &t);
+            if (rc) goto err;
 
-        state = libxl__domain_pvcontrol_read(gc, t, domid);
+            state = libxl__domain_pvcontrol_read(gc, t, domid);
 
-        if (!domain_suspend_pvcontrol_acked(state))
-            libxl__domain_pvcontrol_write(gc, t, domid, "");
+            if (!domain_suspend_pvcontrol_acked(state))
+                libxl__domain_pvcontrol_write(gc, t, domid, "");
 
-        if (!xs_transaction_end(CTX->xsh, t, 0))
-            if (errno == EAGAIN)
-                goto retry_transaction;
+            rc = libxl__xs_transaction_commit(gc, &t);
+            if (!rc) break;
+            if (rc<0) goto err;
+        }
     }
 
     /*
-- 
1.7.10.4

[PATCH 19/23] libxl: suspend: Async xenstore pvcontrol wait

[Ian Jackson]

When negotiating guest suspend via the xenstore pvcontrol protocol
(ie when the guest does NOT support the evtchn fast suspend protocol):

Replace the use of loops and usleep with a call to libxl__xswait.

Also, replace the xenstore transaction loop with one using
libxl__xs_transaction_start et al.

There is not intended to be any semantic change, other than to make
the algorithm properly asynchronous.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_dom.c      |   93 ++++++++++++++++++++++++++----------------
 tools/libxl/libxl_internal.h |    1 +
 2 files changed, 59 insertions(+), 35 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 8aceba9..dde7e33 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1028,6 +1028,8 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
                                              libxl__domain_suspend_state *dss);
 static void domain_suspend_common_guest_suspended(libxl__egc *egc,
                                          libxl__domain_suspend_state *dss);
+static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
+      libxl__xswait_state *xswa, int rc, const char *state);
 static void domain_suspend_common_failed(libxl__egc *egc,
                                          libxl__domain_suspend_state *dss);
 static void domain_suspend_common_done(libxl__egc *egc,
@@ -1047,10 +1049,6 @@ static void domain_suspend_callback_common(libxl__egc *egc,
     STATE_AO_GC(dss->ao);
     unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
     int ret;
-    char *state = "suspend";
-    int watchdog;
-    xs_transaction_t t;
-    int rc;
 
     /* Convenience aliases */
     const uint32_t domid = dss->domid;
@@ -1096,59 +1094,81 @@ static void domain_suspend_callback_common(libxl__egc *egc,
 
     libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "suspend");
 
-    LOG(DEBUG, "wait for the guest to acknowledge suspend request");
-    watchdog = 60;
-    while (!domain_suspend_pvcontrol_acked(state) && watchdog > 0) {
-        usleep(100000);
+    dss->pvcontrol.path = libxl__domain_pvcontrol_xspath(gc, domid);
+    if (!dss->pvcontrol.path) goto err;
 
-        state = libxl__domain_pvcontrol_read(gc, XBT_NULL, domid);
+    dss->pvcontrol.ao = ao;
+    dss->pvcontrol.what = "guest acknowledgement of suspend request";
+    dss->pvcontrol.timeout_ms = 60 * 1000;
+    dss->pvcontrol.callback = domain_suspend_common_pvcontrol_suspending;
+    libxl__xswait_start(gc, &dss->pvcontrol);
+    return;
 
-        watchdog--;
-    }
+ err:
+    domain_suspend_common_failed(egc, dss);
+}
 
-    /*
-     * Guest appears to not be responding. Cancel the suspend
-     * request.
-     *
-     * We re-read the suspend node and clear it within a
-     * transaction in order to handle the case where we race
-     * against the guest catching up and acknowledging the request
-     * at the last minute.
-     */
-    if (!domain_suspend_pvcontrol_acked(state)) {
-        LOG(ERROR, "guest didn't acknowledge suspend, cancelling request");
+static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
+      libxl__xswait_state *xswa, int rc, const char *state)
+{
+    libxl__domain_suspend_state *dss = CONTAINER_OF(xswa, *dss, pvcontrol);
+    STATE_AO_GC(dss->ao);
+    xs_transaction_t t = 0;
+
+    if (!rc && !domain_suspend_pvcontrol_acked(state))
+        /* keep waiting */
+        return;
+
+    libxl__xswait_stop(gc, &dss->pvcontrol);
+
+    if (rc == ERROR_TIMEDOUT) {
+        /*
+         * Guest appears to not be responding. Cancel the suspend
+         * request.
+         *
+         * We re-read the suspend node and clear it within a
+         * transaction in order to handle the case where we race
+         * against the guest catching up and acknowledging the request
+         * at the last minute.
+         */
         for (;;) {
             rc = libxl__xs_transaction_start(gc, &t);
             if (rc) goto err;
 
-            state = libxl__domain_pvcontrol_read(gc, t, domid);
+            rc = libxl__xs_read_checked(gc, t, xswa->path, &state);
+            if (rc) goto err;
+
+            if (domain_suspend_pvcontrol_acked(state))
+                break;
 
-            if (!domain_suspend_pvcontrol_acked(state))
-                libxl__domain_pvcontrol_write(gc, t, domid, "");
+            rc = libxl__xs_write_checked(gc, t, xswa->path, "");
+            if (rc) goto err;
 
             rc = libxl__xs_transaction_commit(gc, &t);
-            if (!rc) break;
+            if (!rc) {
+                LOG(ERROR,
+                    "guest didn't acknowledge suspend, cancelling request");
+                goto err;
+            }
             if (rc<0) goto err;
         }
-    }
-
-    /*
-     * Final check for guest acknowledgement. The guest may have
-     * acknowledged while we were cancelling the request in which
-     * case we lost the race while cancelling and should continue.
-     */
-    if (!domain_suspend_pvcontrol_acked(state)) {
-        LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
+    } else if (rc) {
+        /* some error in xswait's read of xenstore, already logged */
         goto err;
     }
 
+    assert(domain_suspend_pvcontrol_acked(state));
     LOG(DEBUG, "guest acknowledged suspend request");
+
+    libxl__xs_transaction_abort(gc, &t);
     dss->guest_responded = 1;
     domain_suspend_common_wait_guest(egc,dss);
     return;
 
  err:
+    libxl__xs_transaction_abort(gc, &t);
     domain_suspend_common_failed(egc, dss);
+    return;
 }
 
 static void domain_suspend_common_wait_guest(libxl__egc *egc,
@@ -1215,6 +1235,8 @@ static void domain_suspend_common_done(libxl__egc *egc,
                                        libxl__domain_suspend_state *dss,
                                        bool ok)
 {
+    EGC_GC;
+    assert(!libxl__xswait_inuse(&dss->pvcontrol));
     dss->callback_common_done(egc, dss, ok);
 }
 
@@ -1400,6 +1422,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
         &dss->shs.callbacks.save.a;
 
     logdirty_init(&dss->logdirty);
+    libxl__xswait_init(&dss->pvcontrol);
 
     switch (type) {
     case LIBXL_DOMAIN_TYPE_HVM: {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 93e0d4e..c4a509b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2410,6 +2410,7 @@ struct libxl__domain_suspend_state {
     int hvm;
     int xcflags;
     int guest_responded;
+    libxl__xswait_state pvcontrol;
     const char *dm_savefile;
     int interval; /* checkpoint interval (for Remus) */
     libxl__save_helper_state shs;
-- 
1.7.10.4

[PATCH 20/23] libxl: suspend: Abolish usleeps in domain suspend wait

[Ian Jackson]

Replace the use of a loop with usleep().

Instead, use a xenstore watch and an event system timeout.  xenstore
fires watches on @releaseDomain when a domain shuts down.

The logic which checks for the state of the domain is unchanged, and
not ideal, but we will leave that for the next patch.

There is not intended to be any semantic change, other than to make
the algorithm properly asynchronous and the consequential waiting on
xenstore, rather than polling.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_dom.c      |   80 ++++++++++++++++++++++++++++++------------
 tools/libxl/libxl_internal.h |    2 ++
 2 files changed, 60 insertions(+), 22 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index dde7e33..6291115 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1028,8 +1028,14 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
                                              libxl__domain_suspend_state *dss);
 static void domain_suspend_common_guest_suspended(libxl__egc *egc,
                                          libxl__domain_suspend_state *dss);
+
 static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
       libxl__xswait_state *xswa, int rc, const char *state);
+static void suspend_common_wait_guest_watch(libxl__egc *egc,
+      libxl__ev_xswatch *xsw, const char *watch_path, const char *event_path);
+static void suspend_common_wait_guest_timeout(libxl__egc *egc,
+      libxl__ev_time *ev, const struct timeval *requested_abs);
+
 static void domain_suspend_common_failed(libxl__egc *egc,
                                          libxl__domain_suspend_state *dss);
 static void domain_suspend_common_done(libxl__egc *egc,
@@ -1175,36 +1181,59 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
                                              libxl__domain_suspend_state *dss)
 {
     STATE_AO_GC(dss->ao);
+    int rc;
+
+    LOG(DEBUG, "wait for the guest to suspend");
+
+    rc = libxl__ev_xswatch_register(gc, &dss->guest_watch,
+                                    suspend_common_wait_guest_watch,
+                                    "@releaseDomain");
+    if (rc) goto err;
+
+    rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout, 
+                                     suspend_common_wait_guest_timeout,
+                                     60*1000);
+    if (rc) goto err;
+    return;
+
+ err:
+    domain_suspend_common_failed(egc, dss);
+}
+
+static void suspend_common_wait_guest_watch(libxl__egc *egc,
+      libxl__ev_xswatch *xsw, const char *watch_path, const char *event_path)
+{
+    libxl__domain_suspend_state *dss =
+        CONTAINER_OF(xsw, *dss, guest_watch);
+    STATE_AO_GC(dss->ao);
+    xc_domaininfo_t info;
     int ret;
-    int watchdog;
 
     /* Convenience aliases */
     const uint32_t domid = dss->domid;
 
-    LOG(DEBUG, "wait for the guest to suspend");
-    watchdog = 60;
-    while (watchdog > 0) {
-        xc_domaininfo_t info;
-
-        usleep(100000);
-        ret = xc_domain_getinfolist(CTX->xch, domid, 1, &info);
-        if (ret == 1 && info.domain == domid &&
-            (info.flags & XEN_DOMINF_shutdown)) {
-            int shutdown_reason;
-
-            shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
-                & XEN_DOMINF_shutdownmask;
-            if (shutdown_reason == SHUTDOWN_suspend) {
-                LOG(DEBUG, "guest has suspended");
-                domain_suspend_common_guest_suspended(egc, dss);
-                return;
-            }
+    ret = xc_domain_getinfolist(CTX->xch, domid, 1, &info);
+    if (ret == 1 && info.domain == domid &&
+        (info.flags & XEN_DOMINF_shutdown)) {
+        int shutdown_reason;
+
+        shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
+            & XEN_DOMINF_shutdownmask;
+        if (shutdown_reason == SHUTDOWN_suspend) {
+            LOG(DEBUG, "guest has suspended");
+            domain_suspend_common_guest_suspended(egc, dss);
+            return;
         }
-
-        watchdog--;
     }
+    /* otherwise, keep waiting */
+}
 
-    LOG(ERROR, "guest did not suspend");
+static void suspend_common_wait_guest_timeout(libxl__egc *egc,
+      libxl__ev_time *ev, const struct timeval *requested_abs)
+{
+    libxl__domain_suspend_state *dss = CONTAINER_OF(ev, *dss, guest_timeout);
+    STATE_AO_GC(dss->ao);
+    LOG(ERROR, "guest did not suspend, timed out");
     domain_suspend_common_failed(egc, dss);
 }
 
@@ -1214,6 +1243,9 @@ static void domain_suspend_common_guest_suspended(libxl__egc *egc,
     STATE_AO_GC(dss->ao);
     int ret;
 
+    libxl__ev_xswatch_deregister(gc, &dss->guest_watch);
+    libxl__ev_time_deregister(gc, &dss->guest_timeout);
+
     if (dss->hvm) {
         ret = libxl__domain_suspend_device_model(gc, dss);
         if (ret) {
@@ -1237,6 +1269,8 @@ static void domain_suspend_common_done(libxl__egc *egc,
 {
     EGC_GC;
     assert(!libxl__xswait_inuse(&dss->pvcontrol));
+    libxl__ev_xswatch_deregister(gc, &dss->guest_watch);
+    libxl__ev_time_deregister(gc, &dss->guest_timeout);
     dss->callback_common_done(egc, dss, ok);
 }
 
@@ -1423,6 +1457,8 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
 
     logdirty_init(&dss->logdirty);
     libxl__xswait_init(&dss->pvcontrol);
+    libxl__ev_xswatch_init(&dss->guest_watch);
+    libxl__ev_time_init(&dss->guest_timeout);
 
     switch (type) {
     case LIBXL_DOMAIN_TYPE_HVM: {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c4a509b..d428e77 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2411,6 +2411,8 @@ struct libxl__domain_suspend_state {
     int xcflags;
     int guest_responded;
     libxl__xswait_state pvcontrol;
+    libxl__ev_xswatch guest_watch;
+    libxl__ev_time guest_timeout;
     const char *dm_savefile;
     int interval; /* checkpoint interval (for Remus) */
     libxl__save_helper_state shs;
-- 
1.7.10.4

[PATCH 21/23] libxl: suspend: Fix suspend wait corner cases

[Ian Jackson]

When we are waiting for a guest to suspend, this suspend operation
would continue to wait (until the timeout) if the guest was destroyed
or shut down for another reason, or if xc_domain_getinfolist failed.

Handle these cases correctly, as errors.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_dom.c |   42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 6291115..88700ee 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1208,24 +1208,42 @@ static void suspend_common_wait_guest_watch(libxl__egc *egc,
     STATE_AO_GC(dss->ao);
     xc_domaininfo_t info;
     int ret;
+    int shutdown_reason;
 
     /* Convenience aliases */
     const uint32_t domid = dss->domid;
 
     ret = xc_domain_getinfolist(CTX->xch, domid, 1, &info);
-    if (ret == 1 && info.domain == domid &&
-        (info.flags & XEN_DOMINF_shutdown)) {
-        int shutdown_reason;
-
-        shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
-            & XEN_DOMINF_shutdownmask;
-        if (shutdown_reason == SHUTDOWN_suspend) {
-            LOG(DEBUG, "guest has suspended");
-            domain_suspend_common_guest_suspended(egc, dss);
-            return;
-        }
+    if (ret < 0) {
+        LOGE(ERROR, "unable to check for status of guest %"PRId32"", domid);
+        goto err;
+        domain_suspend_common_failed(egc, dss);
+    }
+
+    if (!(ret == 1 && info.domain == domid)) {
+        LOGE(ERROR, "guest %"PRId32" we were suspending has been destroyed",
+             domid);
+        goto err;
+    }
+
+    if (!(info.flags & XEN_DOMINF_shutdown))
+        /* keep waiting */
+        return;
+
+    shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
+        & XEN_DOMINF_shutdownmask;
+    if (shutdown_reason != SHUTDOWN_suspend) {
+        LOG(DEBUG, "guest %"PRId32" we were suspending has shut down"
+            " with unexpected reason code %d", domid, shutdown_reason);
+        goto err;
     }
-    /* otherwise, keep waiting */
+
+    LOG(DEBUG, "guest has suspended");
+    domain_suspend_common_guest_suspended(egc, dss);
+    return;
+
+ err:
+    domain_suspend_common_failed(egc, dss);
 }
 
 static void suspend_common_wait_guest_timeout(libxl__egc *egc,
-- 
1.7.10.4

[PATCH 22/23] libxl: suspend: Async evtchn wait

[Ian Jackson]

When negotiating guest suspend via the evtchn ("fast") protocol,
abolish synchronous wait for domain suspend.

If the guest supports the event channel suspend protocol, we used to
sit in a loop waiting for it to suspend.

Instead, use the new libxl event channel event facility.  When we see
that the event is signaled, we look at the domain to see if it has
suspended.

So the suspend operation no longer blocks with the libxl ctx lock
held, and instead returns to the event loop.  Additionally, domains
which signal the event channel themselves, or undergo other state
changes, will be handled more correctly.

We end up making a few more hypercalls.

Also, if we encounter errors setting up the suspend event channel
(which should not happen), abort the operation rather than falling
back to the xenstore protocol.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_dom.c      |   78 ++++++++++++++++++++++++++----------------
 tools/libxl/libxl_internal.h |    3 +-
 2 files changed, 50 insertions(+), 31 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 88700ee..c431a2d 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1031,8 +1031,12 @@ static void domain_suspend_common_guest_suspended(libxl__egc *egc,
 
 static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
       libxl__xswait_state *xswa, int rc, const char *state);
+static void domain_suspend_common_wait_guest_evtchn(libxl__egc *egc,
+        libxl__ev_evtchn *evev);
 static void suspend_common_wait_guest_watch(libxl__egc *egc,
       libxl__ev_xswatch *xsw, const char *watch_path, const char *event_path);
+static void suspend_common_wait_guest_check(libxl__egc *egc,
+        libxl__domain_suspend_state *dss);
 static void suspend_common_wait_guest_timeout(libxl__egc *egc,
       libxl__ev_time *ev, const struct timeval *requested_abs);
 
@@ -1054,7 +1058,7 @@ static void domain_suspend_callback_common(libxl__egc *egc,
 {
     STATE_AO_GC(dss->ao);
     unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
-    int ret;
+    int ret, rc;
 
     /* Convenience aliases */
     const uint32_t domid = dss->domid;
@@ -1064,21 +1068,19 @@ static void domain_suspend_callback_common(libxl__egc *egc,
         xc_get_hvm_param(CTX->xch, domid, HVM_PARAM_ACPI_S_STATE, &hvm_s_state);
     }
 
-    if ((hvm_s_state == 0) && (dss->suspend_eventchn >= 0)) {
+    if ((hvm_s_state == 0) && (dss->guest_evtchn.port >= 0)) {
         LOG(DEBUG, "issuing %s suspend request via event channel",
             dss->hvm ? "PVHVM" : "PV");
-        ret = xc_evtchn_notify(dss->xce, dss->suspend_eventchn);
+        ret = xc_evtchn_notify(CTX->xce, dss->guest_evtchn.port);
         if (ret < 0) {
             LOG(ERROR, "xc_evtchn_notify failed ret=%d", ret);
             goto err;
         }
-        ret = xc_await_suspend(CTX->xch, dss->xce, dss->suspend_eventchn);
-        if (ret < 0) {
-            LOG(ERROR, "xc_await_suspend failed ret=%d", ret);
-            goto err;
-        }
-        dss->guest_responded = 1;
-        domain_suspend_common_guest_suspended(egc, dss);
+
+        dss->guest_evtchn.callback = domain_suspend_common_wait_guest_evtchn;
+        rc = libxl__ev_evtchn_wait(gc, &dss->guest_evtchn);
+        if (rc) goto err;
+
         return;
     }
 
@@ -1114,6 +1116,19 @@ static void domain_suspend_callback_common(libxl__egc *egc,
     domain_suspend_common_failed(egc, dss);
 }
 
+static void domain_suspend_common_wait_guest_evtchn(libxl__egc *egc,
+        libxl__ev_evtchn *evev)
+{
+    libxl__domain_suspend_state *dss = CONTAINER_OF(evev, *dss, guest_evtchn);
+    STATE_AO_GC(dss->ao);
+    /* If we should be done waiting, suspend_common_wait_guest_check
+     * will end up calling domain_suspend_common_guest_suspended or
+     * domain_suspend_common_failed, both of which cancel the evtchn
+     * wait.  So re-enable it now. */
+    libxl__ev_evtchn_wait(gc, &dss->guest_evtchn);
+    suspend_common_wait_guest_check(egc, dss);
+}
+
 static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
       libxl__xswait_state *xswa, int rc, const char *state)
 {
@@ -1203,8 +1218,13 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
 static void suspend_common_wait_guest_watch(libxl__egc *egc,
       libxl__ev_xswatch *xsw, const char *watch_path, const char *event_path)
 {
-    libxl__domain_suspend_state *dss =
-        CONTAINER_OF(xsw, *dss, guest_watch);
+    libxl__domain_suspend_state *dss = CONTAINER_OF(xsw, *dss, guest_watch);
+    suspend_common_wait_guest_check(egc, dss);
+}
+
+static void suspend_common_wait_guest_check(libxl__egc *egc,
+        libxl__domain_suspend_state *dss)
+{
     STATE_AO_GC(dss->ao);
     xc_domaininfo_t info;
     int ret;
@@ -1261,6 +1281,7 @@ static void domain_suspend_common_guest_suspended(libxl__egc *egc,
     STATE_AO_GC(dss->ao);
     int ret;
 
+    libxl__ev_evtchn_cancel(gc, &dss->guest_evtchn);
     libxl__ev_xswatch_deregister(gc, &dss->guest_watch);
     libxl__ev_time_deregister(gc, &dss->guest_timeout);
 
@@ -1287,6 +1308,7 @@ static void domain_suspend_common_done(libxl__egc *egc,
 {
     EGC_GC;
     assert(!libxl__xswait_inuse(&dss->pvcontrol));
+    libxl__ev_evtchn_cancel(gc, &dss->guest_evtchn);
     libxl__ev_xswatch_deregister(gc, &dss->guest_watch);
     libxl__ev_time_deregister(gc, &dss->guest_timeout);
     dss->callback_common_done(egc, dss, ok);
@@ -1475,6 +1497,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
 
     logdirty_init(&dss->logdirty);
     libxl__xswait_init(&dss->pvcontrol);
+    libxl__ev_evtchn_init(&dss->guest_evtchn);
     libxl__ev_xswatch_init(&dss->guest_watch);
     libxl__ev_time_init(&dss->guest_timeout);
 
@@ -1503,7 +1526,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
           | (debug ? XCFLAGS_DEBUG : 0)
           | (dss->hvm ? XCFLAGS_HVM : 0);
 
-    dss->suspend_eventchn = -1;
+    dss->guest_evtchn.port = -1;
     dss->guest_evtchn_lockfd = -1;
     dss->guest_responded = 0;
     dss->dm_savefile = libxl__device_model_savefile(gc, domid);
@@ -1514,20 +1537,17 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
             dss->xcflags |= XCFLAGS_CHECKPOINT_COMPRESS;
     }
 
-    dss->xce = xc_evtchn_open(NULL, 0);
-    if (dss->xce == NULL)
-        goto out;
-    else
-    {
-        port = xs_suspend_evtchn_port(dss->domid);
+    port = xs_suspend_evtchn_port(dss->domid);
 
-        if (port >= 0) {
-            dss->suspend_eventchn =
-                xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce,
+    if (port >= 0) {
+        dss->guest_evtchn.port =
+            xc_suspend_evtchn_init_exclusive(CTX->xch, CTX->xce,
                                   dss->domid, port, &dss->guest_evtchn_lockfd);
 
-            if (dss->suspend_eventchn < 0)
-                LOG(WARN, "Suspend event channel initialization failed");
+        if (dss->guest_evtchn.port < 0) {
+            LOG(WARN, "Suspend event channel initialization failed");
+            rc = ERROR_FAIL;
+            goto out;
         }
     }
 
@@ -1686,11 +1706,11 @@ static void domain_suspend_done(libxl__egc *egc,
     /* Convenience aliases */
     const uint32_t domid = dss->domid;
 
-    if (dss->suspend_eventchn > 0)
-        xc_suspend_evtchn_release(CTX->xch, dss->xce, domid,
-                           dss->suspend_eventchn, &dss->guest_evtchn_lockfd);
-    if (dss->xce != NULL)
-        xc_evtchn_close(dss->xce);
+    libxl__ev_evtchn_cancel(gc, &dss->guest_evtchn);
+
+    if (dss->guest_evtchn.port > 0)
+        xc_suspend_evtchn_release(CTX->xch, CTX->xce, domid,
+                           dss->guest_evtchn.port, &dss->guest_evtchn_lockfd);
 
     dss->callback(egc, dss, rc);
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d428e77..126e5bf 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2404,8 +2404,7 @@ struct libxl__domain_suspend_state {
     int debug;
     const libxl_domain_remus_info *remus;
     /* private */
-    xc_evtchn *xce; /* event channel handle */
-    int suspend_eventchn;
+    libxl__ev_evtchn guest_evtchn;
     int guest_evtchn_lockfd;
     int hvm;
     int xcflags;
-- 
1.7.10.4

[PATCH 23/23] libxl: suspend: Apply guest timeout in evtchn case

[Ian Jackson]

When negotiating guest suspend via the evtchn ("fast") protocol,
the guest may still fail to respond.

So set the timeout.  The existing error path will already properly
tear down our (event channel) wait.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_dom.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c431a2d..820ec3a 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1081,6 +1081,11 @@ static void domain_suspend_callback_common(libxl__egc *egc,
         rc = libxl__ev_evtchn_wait(gc, &dss->guest_evtchn);
         if (rc) goto err;
 
+        rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout,
+                                         suspend_common_wait_guest_timeout,
+                                         60*1000);
+        if (rc) goto err;
+
         return;
     }
 
-- 
1.7.10.4

Re: (no subject)

[George Dunlap]

On 12/17/2013 06:35 PM, Ian Jackson wrote:
> This series removes the usleeps and waiting loops in libxl_dom.c and
> replaces them with event-callback code.
>
> Firstly, some documentation of things I had to reverse-engineer:
>   01/23 xen: Document XEN_DOMCTL_subscribe
>   02/23 xen: Document that EVTCHNOP_bind_interdomain signals
>   03/23 docs: Document event-channel-based suspend protocol
>   04/23 libxc: Document xenctrl.h event channel calls
> Arguably these might be 4.4 material (hence the CC to George).

These document pretty important aspects of behavior (fixing what might 
be considered documentation bugs), and obviously can have no functional 
impact.  I guess the only risk is that they might be wrong and mislead 
someone, but I think that's pretty low. :-)

These four:

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: (no subject)

[Ian Campbell]

On Wed, 2013-12-18 at 11:19 +0000, George Dunlap wrote:
> On 12/17/2013 06:35 PM, Ian Jackson wrote:
> > This series removes the usleeps and waiting loops in libxl_dom.c and
> > replaces them with event-callback code.
> >
> > Firstly, some documentation of things I had to reverse-engineer:
> >   01/23 xen: Document XEN_DOMCTL_subscribe
> >   02/23 xen: Document that EVTCHNOP_bind_interdomain signals
> >   03/23 docs: Document event-channel-based suspend protocol
> >   04/23 libxc: Document xenctrl.h event channel calls
> > Arguably these might be 4.4 material (hence the CC to George).
> 
> These document pretty important aspects of behavior (fixing what might 
> be considered documentation bugs), and obviously can have no functional 
> impact.  I guess the only risk is that they might be wrong and mislead 
> someone, but I think that's pretty low. :-)
> 
> These four:
> 
> Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

They look good to me too:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 05/23] libxl: init: Provide a gc later in libxl_ctx_alloc

[Ian Campbell]

On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> Provide libxl__gc *gc for the second half of libxl_ctx_alloc.
> (For the first half of the function, gc is in scope but set to NULL.)

libxl uses -Wno-declaration-after-statement so it would be valid to just
declare it at the point it becomes valid, would avoid mistakes with
people adding uses before this point.

> This makes it possible to make gc-requiring calls.  For example, it
> makes error logging more convenient.
> 
> Make use of this by changing the logging calls to use the LOG*
> convenience macros.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index e8ad610..cfd1435 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -25,6 +25,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>                      unsigned flags, xentoollog_logger * lg)
>  {
>      libxl_ctx *ctx = NULL;
> +    libxl__gc gc_buf, *gc = NULL;
>      int rc;
>  
>      if (version != LIBXL_VERSION) { rc = ERROR_VERSION; goto out; }
> @@ -79,6 +80,9 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>      }
>  
>      /* Now ctx is safe for ctx_free; failures simply set rc and "goto out" */
> +    LIBXL_INIT_GC(gc_buf,ctx);
> +    gc = &gc_buf;
> +    /* Now gc is useable */
>  
>      rc = libxl__atfork_init(ctx);
>      if (rc) goto out;
> @@ -88,8 +92,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>  
>      ctx->xch = xc_interface_open(lg,lg,0);
>      if (!ctx->xch) {
> -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
> -                        "cannot open libxc handle");
> +        LOGEV(ERROR, errno, "cannot open libxc handle");
>          rc = ERROR_FAIL; goto out;
>      }
>  
> @@ -97,8 +100,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>      if (!ctx->xsh)
>          ctx->xsh = xs_domain_open();
>      if (!ctx->xsh) {
> -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
> -                        "cannot connect to xenstore");
> +        LOGEV(ERROR, errno, "cannot connect to xenstore");
>          rc = ERROR_FAIL; goto out;
>      }
>  
> @@ -106,6 +108,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>      return 0;
>  
>   out:
> +    if (gc) libxl__free_all(gc);
>      libxl_ctx_free(ctx);
>      *pctx = NULL;
>      return rc;

Re: [PATCH 06/23] libxl: init: libxl__poller_init and _get take gc

[Ian Campbell]

On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> @@ -1622,7 +1617,7 @@ libxl__ao *libxl__ao_create(libxl_ctx *ctx, uint32_t domid,
>      if (how) {
>          ao->how = *how;
>      } else {
> -        ao->poller = libxl__poller_get(ctx);
> +        ao->poller = libxl__poller_get(&ao->gc);

this is a bit subtle, libxl__poller_get would previously have gotten the
toplevel ctx gc and not the ao-gc, if it had thought to init a GC at
all.

It only actually uses whatever it gets for logging, so that's ok, but
I'm not sure which would be the correct/expected gc to use. I take it
you considered this?

libxl__poller_get does a NOGC allocation and the associated manual
memory mgmt -- now that it has the ao->gc in hand could it use that? Is
the poller's lifecycle entirely contained within the ao?

Ian.

Re: [PATCH 07/23] libxl: events: const-correct *_inuse, *_isregistered

[Ian Campbell]

On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> The comments for libxl__ev_time_isregistered and the corresponding
> watch function even say that these should be const.  Make it so.
> 
> Also fix libxl__ev_child_inuse and libxl__ev_spawn_inuse.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  tools/libxl/libxl_internal.h |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 2712005..51d6c6d 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -716,7 +716,7 @@ _hidden int libxl__ev_fd_modify(libxl__gc*, libxl__ev_fd *ev,
>  _hidden void libxl__ev_fd_deregister(libxl__gc*, libxl__ev_fd *ev);
>  static inline void libxl__ev_fd_init(libxl__ev_fd *efd)
>                      { efd->fd = -1; }
> -static inline int libxl__ev_fd_isregistered(libxl__ev_fd *efd)
> +static inline int libxl__ev_fd_isregistered(const libxl__ev_fd *efd)
>                      { return efd->fd >= 0; }
>  
>  _hidden int libxl__ev_time_register_rel(libxl__gc*, libxl__ev_time *ev_out,
> @@ -732,7 +732,7 @@ _hidden int libxl__ev_time_modify_abs(libxl__gc*, libxl__ev_time *ev,
>  _hidden void libxl__ev_time_deregister(libxl__gc*, libxl__ev_time *ev);
>  static inline void libxl__ev_time_init(libxl__ev_time *ev)
>                  { ev->func = 0; }
> -static inline int libxl__ev_time_isregistered(libxl__ev_time *ev)
> +static inline int libxl__ev_time_isregistered(const libxl__ev_time *ev)
>                  { return !!ev->func; }
>  
> 
> @@ -772,7 +772,7 @@ _hidden pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child *childw_out,
>                                   libxl__ev_child_callback *death);
>  static inline void libxl__ev_child_init(libxl__ev_child *childw_out)
>                  { childw_out->pid = -1; }
> -static inline int libxl__ev_child_inuse(libxl__ev_child *childw_out)
> +static inline int libxl__ev_child_inuse(const libxl__ev_child *childw_out)
>                  { return childw_out->pid >= 0; }
>  
>  /* Useable (only) in the child to once more make the ctx useable for
> @@ -1213,7 +1213,7 @@ struct libxl__spawn_state {
>      libxl__ev_xswatch xswatch;
>  };
>  
> -static inline int libxl__spawn_inuse(libxl__spawn_state *ss)
> +static inline int libxl__spawn_inuse(const libxl__spawn_state *ss)
>      { return libxl__ev_child_inuse(&ss->mid); }
>  
>  /*

Re: [PATCH 08/23] libxl: events: Provide libxl__xswait_*

[Ian Campbell]

On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> +/*----- xswait: wait for a xenstore node to be suitable -----*/
> +
> +typedef struct libxl__xswait_state libxl__xswait_state;
> +
> +/*
> + * rc describes the circumstances of this callback:
> + *
> + * rc==0
> + *
> + *     The xenstore path (may have) changed.  It has been read for
> + *     you.  The result is in data (allocated from the ao gc).
> + *     data may be NULL, which means that the xenstore read gave
> + *     ENOENT.
> + *
> + *     If you are satisfied, you MUST call libxl__xswait_stop.
> + *     Otherwise, xswait will continue waiting and watching and
> + *     will call you back later.
> + *
> + * rc==ETIMEDOUT

Isn't rc actual a libxl error and hence ERROR_TIMEDOUT?

> + *
> + *     The specified timeout was reached.
> + *     This has NOT been logged (except to the debug log).
> + *     xswait will not continue (but calling libxl__xswait_stop is OK).
> + *
> + * rc!=0

also != E{,RROR_}TIMEDOUT </pedant>

> + *
> + *     Some other error occurred.
> + *     This HAS been logged.
> + *     xswait will not continue (but calling libxl__xswait_stop is OK).
> + *
> + */
> +typedef void libxl__xswait_callback(libxl__egc *egc,
> +      libxl__xswait_state *xswa, int rc, const char *data);
> +
> +struct libxl__xswait_state {
> +    /* caller must fill these in, and they must all remain valid */
> +    libxl__ao *ao;
> +    const char *what; /* for error msgs: noun phrase, what we're waiting for */
> +    const char *path;
> +    int timeout_ms; /* as for poll(2) */
> +    libxl__xswait_callback *callback;
> +    /* remaining fields are private to xswait */
> +    libxl__ev_time time_ev;
> +    libxl__ev_xswatch watch_ev;
> +};
> +
> +void libxl__xswait_init(libxl__xswait_state*);
> +void libxl__xswait_stop(libxl__gc*, libxl__xswait_state*); /*idempotent*/
> +bool libxl__xswait_inuse(const libxl__xswait_state *ss);
> +
> +int libxl__xswait_start(libxl__gc*, libxl__xswait_state*);
> +
>  /*
>   *----- spawn -----
>   *

Re: [PATCH 09/23] libxl: events: Use libxl__xswait_* in spawn code

[Ian Campbell]

On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> Replace open-coded use of ev_time and ev_xswatch with xswait.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

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

[Ian Campbell]

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

Re: [PATCH 05/23] libxl: init: Provide a gc later in libxl_ctx_alloc

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 05/23] libxl: init: Provide a gc later in libxl_ctx_alloc"):
> On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> > Provide libxl__gc *gc for the second half of libxl_ctx_alloc.
> > (For the first half of the function, gc is in scope but set to NULL.)
> 
> libxl uses -Wno-declaration-after-statement so it would be valid to just
> declare it at the point it becomes valid, would avoid mistakes with
> people adding uses before this point.

However, that would involve changing the "goto out" in the malloc
failure case to something else, because "goto out" would jump past the
initialisation of gc to NULL.

The malloc failure case can't be handled with our usual fatal error
logic because that depends on having a ctx.  I guess we could have a
dummy ctx but it would get fiddly.

Ian.

Re: [PATCH 06/23] libxl: init: libxl__poller_init and _get take gc

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 06/23] libxl: init: libxl__poller_init and _get take gc"):
> On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> > @@ -1622,7 +1617,7 @@ libxl__ao *libxl__ao_create(libxl_ctx *ctx, uint32_t domid,
> >      if (how) {
> >          ao->how = *how;
> >      } else {
> > -        ao->poller = libxl__poller_get(ctx);
> > +        ao->poller = libxl__poller_get(&ao->gc);
> 
> this is a bit subtle, libxl__poller_get would previously have gotten the
> toplevel ctx gc and not the ao-gc, if it had thought to init a GC at
> all.

Yes.

> It only actually uses whatever it gets for logging, so that's ok, but
> I'm not sure which would be the correct/expected gc to use. I take it
> you considered this?

Indeed.

> libxl__poller_get does a NOGC allocation and the associated manual
> memory mgmt -- now that it has the ao->gc in hand could it use that? Is
> the poller's lifecycle entirely contained within the ao?

No, it isn't.  The poller is longer-lived, mainly as a way to avoid
having to recreate the pipe on each occasion.

Ian.

Re: [PATCH 08/23] libxl: events: Provide libxl__xswait_*

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 08/23] libxl: events: Provide libxl__xswait_*"):
> On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> > +/*----- xswait: wait for a xenstore node to be suitable -----*/
> > + *     If you are satisfied, you MUST call libxl__xswait_stop.
> > + *     Otherwise, xswait will continue waiting and watching and
> > + *     will call you back later.
> > + *
> > + * rc==ETIMEDOUT
> 
> Isn't rc actual a libxl error and hence ERROR_TIMEDOUT?

Err, yes.

> > + *
> > + *     The specified timeout was reached.
> > + *     This has NOT been logged (except to the debug log).
> > + *     xswait will not continue (but calling libxl__xswait_stop is OK).
> > + *
> > + * rc!=0
> 
> also != E{,RROR_}TIMEDOUT </pedant>

Both will be fixed in v2.

Ian.

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

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 10/23] libxl: events: Provide libxl__ev_evtchn*"):
> 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.

Yes.

> > +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)

Fixed.

> > [...]
> > +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?

Leaving it as "int" means that the caller can put -1 in it if the
struct isn't in use or the port not allocated.  That avoids the caller
needing to have a separate copy of the value, or a separate boolean.
And later, we do.

> > @@ -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?

The latter.  I'm open to suggestions for improved wording.

> > +    LIBXL_LIST_HEAD(, libxl__ev_evtchn) evtchns_waiting;
> 
> Are there any locking requirements relating to this list?

They are the same as all the other event-related data structures in
the ctx: the ctx mutex protects them.  This isn't particularly
explicit here but I think it's fairly obvious.  Only the event code
looks at this list anyway and it all takes a gc.

> >  /*
> > + * The evtchn facility is one-shot per call t libxl__ev_evtchn_wait.
> 
> s/ t / to /

Fixed.

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

It would be possible to improve the data structure and event code to
support this.  The obvious way would be to keep scanning the
evtchns_waiting list after we found one waiter, but you'd have to do
something fiddly to avoid reentering the same waiter again right away.

Thanks,
Ian.

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

[Ian Campbell]

On Thu, 2013-12-19 at 17:47 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 10/23] libxl: events: Provide 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?
> 
> Leaving it as "int" means that the caller can put -1 in it if the
> struct isn't in use or the port not allocated.  That avoids the caller
> needing to have a separate copy of the value, or a separate boolean.
> And later, we do.

I suppose that's vaguely the same sort of thing as
evtchn_port_or_error_t, but not quite.

> 
> > > @@ -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?
> 
> The latter.  I'm open to suggestions for improved wording.

"waiting must only be done via libxl__ev_..." not much better though.

> 
> > > +    LIBXL_LIST_HEAD(, libxl__ev_evtchn) evtchns_waiting;
> > 
> > Are there any locking requirements relating to this list?
> 
> They are the same as all the other event-related data structures in
> the ctx: the ctx mutex protects them.  This isn't particularly
> explicit here but I think it's fairly obvious.  Only the event code
> looks at this list anyway and it all takes a gc.

OK.

> > > + * 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.
> 
> It would be possible to improve the data structure and event code to
> support this.  The obvious way would be to keep scanning the
> evtchns_waiting list after we found one waiter, but you'd have to do
> something fiddly to avoid reentering the same waiter again right away.

Yes, easiest to avoid until the requirement occurs I think.

Ian.

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

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 10/23] libxl: events: Provide libxl__ev_evtchn*"):
> On Thu, 2013-12-19 at 17:47 +0000, Ian Jackson wrote:
> > Leaving it as "int" means that the caller can put -1 in it if the
> > struct isn't in use or the port not allocated.  That avoids the caller
> > needing to have a separate copy of the value, or a separate boolean.
> > And later, we do.
> 
> I suppose that's vaguely the same sort of thing as
> evtchn_port_or_error_t, but not quite.

Yes.

> > The latter.  I'm open to suggestions for improved wording.
> 
> "waiting must only be done via libxl__ev_..." not much better though.

I will change it to:

    xc_evtchn *xce; /* waiting must be done only with libxl__ev_evtchn* */
    
Thanks,
Ian.

Re: (no subject)

[Ian Campbell]

On Wed, 2013-12-18 at 13:35 +0000, Ian Campbell wrote:
> On Wed, 2013-12-18 at 11:19 +0000, George Dunlap wrote:
> > On 12/17/2013 06:35 PM, Ian Jackson wrote:
> > > This series removes the usleeps and waiting loops in libxl_dom.c and
> > > replaces them with event-callback code.
> > >
> > > Firstly, some documentation of things I had to reverse-engineer:
> > >   01/23 xen: Document XEN_DOMCTL_subscribe
> > >   02/23 xen: Document that EVTCHNOP_bind_interdomain signals
> > >   03/23 docs: Document event-channel-based suspend protocol
> > >   04/23 libxc: Document xenctrl.h event channel calls
> > > Arguably these might be 4.4 material (hence the CC to George).
> > 
> > These document pretty important aspects of behavior (fixing what might 
> > be considered documentation bugs), and obviously can have no functional 
> > impact.  I guess the only risk is that they might be wrong and mislead 
> > someone, but I think that's pretty low. :-)
> > 
> > These four:
> > 
> > Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> They look good to me too:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

And I've now applied just those 4 docs changes.

Ian.

Re: [PATCH 11/23] libxc: suspend: Rename, improve xc_suspend_evtchn_init

[Ian Campbell]

On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> xc_suspend_evtchn_init expects to eat the first event on the xce.  If
> the xce is used for any other purpose then this can break.  Document
> this fact and rename the function to xc_suspend_evtchn_init_exclusive.
> (I haven't checked the call sites for improper shared use of the xce.)
> 
> Provide a corresponding xc_suspend_evtchn_init_sane which doesn't try
> to eat an event, and instead leaves the caller the ability to
> demultiplex.

I'd much rather have fixed the existing one (and its callers as
necessary) but I can see why you wouldn't have the appetite for that.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> Also document that xc_await_suspend needs exclusive use of the xce.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
>  tools/libxc/xc_suspend.c                           |   21 ++++++++++++++++----
>  tools/libxc/xenguest.h                             |   13 +++++++++++-
>  tools/libxl/libxl_dom.c                            |    2 +-
>  tools/misc/xen-hptool.c                            |    2 +-
>  .../python/xen/lowlevel/checkpoint/libcheckpoint.c |    2 +-
>  tools/xcutils/xc_save.c                            |    2 +-
>  6 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
> index 1ace411..7968a44 100644
> --- a/tools/libxc/xc_suspend.c
> +++ b/tools/libxc/xc_suspend.c
> @@ -14,6 +14,8 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
>   */
>  
> +#include <assert.h>
> +
>  #include "xc_private.h"
>  #include "xenguest.h"
>  
> @@ -102,7 +104,7 @@ int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int
>      return unlock_suspend_event(xch, domid);
>  }
>  
> -int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int port)
> +int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port)
>  {
>      int rc, suspend_evtchn = -1;
>  
> @@ -121,9 +123,6 @@ int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int por
>          goto cleanup;
>      }
>  
> -    /* event channel is pending immediately after binding */
> -    xc_await_suspend(xch, xce, suspend_evtchn);
> -
>      return suspend_evtchn;
>  
>  cleanup:
> @@ -132,3 +131,17 @@ cleanup:
>  
>      return -1;
>  }
> +
> +int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port)
> +{
> +    int suspend_evtchn;
> +
> +    suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port);
> +    if (suspend_evtchn < 0)
> +        return suspend_evtchn;
> +
> +    /* event channel is pending immediately after binding */
> +    xc_await_suspend(xch, xce, suspend_evtchn);
> +
> +    return suspend_evtchn;
> +}
> diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
> index a0e30e1..ce5456c 100644
> --- a/tools/libxc/xenguest.h
> +++ b/tools/libxc/xenguest.h
> @@ -256,10 +256,21 @@ int xc_hvm_build_target_mem(xc_interface *xch,
>  
>  int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn);
>  
> -int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int port);
> +/**
> + * This function eats the initial notification.
> + * xce must not be used for anything else
> + */
> +int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port);
>  
> +/* xce must not be used for anything else */
>  int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn);
>  
> +/**
> + * The port will be signaled immediately after this call
> + * The caller should check the domain status and look for the next event
> + */
> +int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port);
> +
>  int xc_get_bit_size(xc_interface *xch,
>                      const char *image_name, const char *cmdline,
>                      const char *features, int *type);
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 55f74b2..4b42856 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1358,7 +1358,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
>  
>          if (port >= 0) {
>              dss->suspend_eventchn =
> -                xc_suspend_evtchn_init(CTX->xch, dss->xce, dss->domid, port);
> +                xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce, dss->domid, port);
>  
>              if (dss->suspend_eventchn < 0)
>                  LOG(WARN, "Suspend event channel initialization failed");
> diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
> index 24c3e95..db76f79 100644
> --- a/tools/misc/xen-hptool.c
> +++ b/tools/misc/xen-hptool.c
> @@ -112,7 +112,7 @@ static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtc
>          fprintf(stderr, "DOM%d: No suspend port, try live migration\n", domid);
>          goto failed;
>      }
> -    suspend_evtchn = xc_suspend_evtchn_init(xch, xce, domid, port);
> +    suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid, port);
>      if (suspend_evtchn < 0)
>      {
>          fprintf(stderr, "Suspend evtchn initialization failed\n");
> diff --git a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> index 01c0d47..817d272 100644
> --- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> +++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> @@ -360,7 +360,7 @@ static int setup_suspend_evtchn(checkpoint_state* s)
>      return -1;
>    }
>  
> -  s->suspend_evtchn = xc_suspend_evtchn_init(s->xch, s->xce, s->domid, port);
> +  s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce, s->domid, port);
>    if (s->suspend_evtchn < 0) {
>        s->errstr = "failed to bind suspend event channel";
>        return -1;
> diff --git a/tools/xcutils/xc_save.c b/tools/xcutils/xc_save.c
> index e34bd2c..974f706 100644
> --- a/tools/xcutils/xc_save.c
> +++ b/tools/xcutils/xc_save.c
> @@ -202,7 +202,7 @@ main(int argc, char **argv)
>          else
>          {
>              si.suspend_evtchn =
> -              xc_suspend_evtchn_init(si.xch, si.xce, si.domid, port);
> +              xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid, port);
>  
>              if (si.suspend_evtchn < 0)
>                  warnx("suspend event channel initialization failed, "