From: Ian Jackson <ian.jackson@eu.citrix.com>
To: xen-devel@lists.xensource.com
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>,
Ian Campbell <ian.campbell@citrix.com>
Subject: [PATCH 08/14] libxl: events: Permit timeouts to signal cancellation
Date: Fri, 20 Dec 2013 18:45:46 +0000 [thread overview]
Message-ID: <1387565152-5642-9-git-send-email-ian.jackson@eu.citrix.com> (raw)
In-Reply-To: <1387565152-5642-1-git-send-email-ian.jackson@eu.citrix.com>
We make these semantic changes to the interface to libxl__ev_time:
* The callback functions provided by users must take an rc value.
This rc value can be ERROR_TIMEDOUT or ERROR_CANCELLED.
* The setup functions take a libxl__ao, not a libxl__gc. This means
that timeouts can only occur as part of a long-running libxl
function (but this is of course correct, as libxl shouldn't have
any global timeouts, and indeed all the call sites have an ao).
The consequential changes at the points where timeouts are used are
generally obvious. However:
* device_hotplug_timeout_cb/device_hotplug_child_death_cb need to
take a bit more care to preserve the error code passed to the
timeout.
* Users of xswait are now expected to deal correctly with
ERROR_CANCELLED. If they experience this, it hasn't been logged.
And the caller won't log it either since it's not TIMEDOUT.
Luckily this is correct, so we can just change the doc comment.
Currently nothing generates ERROR_CANCELLED; in particular the
timeouts cannot in fact signal cancellation.
There should be no publicly visible change except that some error
returns from libxl will change from ERROR_FAIL to ERROR_TIMEDOUT, and
some changes to debugging messages.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/libxl/libxl_aoutils.c | 7 ++++---
tools/libxl/libxl_device.c | 24 ++++++++++++++++--------
tools/libxl/libxl_dom.c | 23 ++++++++++++++---------
tools/libxl/libxl_event.c | 10 ++++++----
tools/libxl/libxl_internal.h | 16 +++++++++-------
5 files changed, 49 insertions(+), 31 deletions(-)
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 477717b..34a3305 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -46,7 +46,7 @@ int libxl__xswait_start(libxl__gc *gc, libxl__xswait_state *xswa)
{
int rc;
- rc = libxl__ev_time_register_rel(gc, &xswa->time_ev,
+ rc = libxl__ev_time_register_rel(xswa->ao, &xswa->time_ev,
xswait_timeout_callback, xswa->timeout_ms);
if (rc) goto err;
@@ -76,12 +76,13 @@ void xswait_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *xsw,
}
void xswait_timeout_callback(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
+ const struct timeval *requested_abs,
+ int rc)
{
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);
+ xswait_report_error(egc, xswa, rc);
}
static void xswait_report_error(libxl__egc *egc, libxl__xswait_state *xswa,
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 2ea95d6..a795671 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -697,7 +697,7 @@ out:
/* This callback is part of the Qemu devices Badge */
static void device_qemu_timeout(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs);
+ const struct timeval *requested_abs, int rc);
static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
int rc);
@@ -708,7 +708,8 @@ static void device_backend_cleanup(libxl__gc *gc,
static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev);
static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs);
+ const struct timeval *requested_abs,
+ int rc);
static void device_hotplug_child_death_cb(libxl__egc *egc,
libxl__ev_child *child,
@@ -790,7 +791,7 @@ void libxl__initiate_device_remove(libxl__egc *egc,
* TODO: 4.2 Bodge due to QEMU, see comment on top of
* libxl__initiate_device_remove in libxl_internal.h
*/
- rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
+ rc = libxl__ev_time_register_rel(ao, &aodev->timeout,
device_qemu_timeout,
LIBXL_QEMU_BODGE_TIMEOUT * 1000);
if (rc) {
@@ -862,7 +863,7 @@ out:
}
static void device_qemu_timeout(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
+ const struct timeval *requested_abs, int rc)
{
libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, timeout);
STATE_AO_GC(aodev->ao);
@@ -870,7 +871,9 @@ static void device_qemu_timeout(libxl__egc *egc, libxl__ev_time *ev,
char *state_path = GCSPRINTF("%s/state", be_path);
const char *xs_state;
xs_transaction_t t = 0;
- int rc = 0;
+
+ if (rc != ERROR_TIMEDOUT)
+ goto out;
libxl__ev_time_deregister(gc, &aodev->timeout);
@@ -998,7 +1001,7 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
}
/* Set hotplug timeout */
- rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
+ rc = libxl__ev_time_register_rel(ao, &aodev->timeout,
device_hotplug_timeout_cb,
LIBXL_HOTPLUG_TIMEOUT * 1000);
if (rc) {
@@ -1035,11 +1038,15 @@ out:
}
static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
+ const struct timeval *requested_abs,
+ int rc)
{
libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, timeout);
STATE_AO_GC(aodev->ao);
+ if (!aodev->rc)
+ aodev->rc = rc;
+
libxl__ev_time_deregister(gc, &aodev->timeout);
assert(libxl__ev_child_inuse(&aodev->child));
@@ -1070,7 +1077,8 @@ static void device_hotplug_child_death_cb(libxl__egc *egc,
GCSPRINTF("%s/hotplug-error", be_path));
if (hotplug_error)
LOG(ERROR, "script: %s", hotplug_error);
- aodev->rc = ERROR_FAIL;
+ if (!aodev->rc)
+ aodev->rc = ERROR_FAIL;
if (aodev->action == LIBXL__DEVICE_ACTION_ADD)
/*
* Only fail on device connection, on disconnection
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 0fda9a4..1b1d06f 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -771,7 +771,8 @@ static void remus_domain_suspend_callback_common_done(libxl__egc *egc,
*/
static void switch_logdirty_timeout(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs);
+ const struct timeval *requested_abs,
+ int rc);
static void switch_logdirty_xswatch(libxl__egc *egc, libxl__ev_xswatch*,
const char *watch_path, const char *event_path);
static void switch_logdirty_done(libxl__egc *egc,
@@ -808,7 +809,7 @@ static void domain_suspend_switch_qemu_xen_traditional_logdirty
switch_logdirty_xswatch, lds->ret_path);
if (rc) goto out;
- rc = libxl__ev_time_register_rel(gc, &lds->timeout,
+ rc = libxl__ev_time_register_rel(ao, &lds->timeout,
switch_logdirty_timeout, 10*1000);
if (rc) goto out;
@@ -897,7 +898,8 @@ void libxl__domain_suspend_common_switch_qemu_logdirty
}
}
static void switch_logdirty_timeout(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
+ const struct timeval *requested_abs,
+ int rc)
{
libxl__domain_suspend_state *dss = CONTAINER_OF(ev, *dss, logdirty.timeout);
STATE_AO_GC(dss->ao);
@@ -1046,7 +1048,7 @@ static void suspend_common_wait_guest_watch(libxl__egc *egc,
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);
+ libxl__ev_time *ev, const struct timeval *requested_abs, int rc);
static void domain_suspend_common_done(libxl__egc *egc,
libxl__domain_suspend_state *dss,
@@ -1088,7 +1090,7 @@ 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,
+ rc = libxl__ev_time_register_rel(ao, &dss->guest_timeout,
suspend_common_wait_guest_timeout,
60*1000);
if (rc) goto err;
@@ -1218,7 +1220,7 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
"@releaseDomain");
if (rc) goto err;
- rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout,
+ rc = libxl__ev_time_register_rel(ao, &dss->guest_timeout,
suspend_common_wait_guest_timeout,
60*1000);
if (rc) goto err;
@@ -1279,12 +1281,15 @@ static void suspend_common_wait_guest_check(libxl__egc *egc,
}
static void suspend_common_wait_guest_timeout(libxl__egc *egc,
- libxl__ev_time *ev, const struct timeval *requested_abs)
+ libxl__ev_time *ev, const struct timeval *requested_abs, int rc)
{
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_done(egc, dss, ERROR_GUEST_TIMEDOUT);
+ if (rc == ERROR_TIMEDOUT) {
+ LOG(ERROR, "guest did not suspend, timed out");
+ rc = ERROR_GUEST_TIMEDOUT;
+ }
+ domain_suspend_common_done(egc, dss, rc);
}
static void domain_suspend_common_guest_suspended(libxl__egc *egc,
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 696c744..28b134a 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -309,10 +309,11 @@ static void time_done_debug(libxl__gc *gc, const char *func,
#endif
}
-int libxl__ev_time_register_abs(libxl__gc *gc, libxl__ev_time *ev,
+int libxl__ev_time_register_abs(libxl__ao *ao, libxl__ev_time *ev,
libxl__ev_time_callback *func,
struct timeval absolute)
{
+ AO_GC;
int rc;
CTX_LOCK;
@@ -333,10 +334,11 @@ int libxl__ev_time_register_abs(libxl__gc *gc, libxl__ev_time *ev,
}
-int libxl__ev_time_register_rel(libxl__gc *gc, libxl__ev_time *ev,
+int libxl__ev_time_register_rel(libxl__ao *ao, libxl__ev_time *ev,
libxl__ev_time_callback *func,
int milliseconds /* as for poll(2) */)
{
+ AO_GC;
struct timeval absolute;
int rc;
@@ -1154,7 +1156,7 @@ static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
etime, (unsigned long)etime->abs.tv_sec,
(unsigned long)etime->abs.tv_usec);
- etime->func(egc, etime, &etime->abs);
+ etime->func(egc, etime, &etime->abs, ERROR_TIMEDOUT);
}
}
@@ -1235,7 +1237,7 @@ void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl)
assert(!ev->infinite);
LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
- ev->func(egc, ev, &ev->abs);
+ ev->func(egc, ev, &ev->abs, ERROR_TIMEDOUT);
out:
CTX_UNLOCK;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index af41200..b2109ea 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -172,7 +172,8 @@ struct libxl__ev_fd {
typedef struct libxl__ev_time libxl__ev_time;
typedef void libxl__ev_time_callback(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs);
+ const struct timeval *requested_abs,
+ int rc); /* TIMEDOUT or CANCELLED */
struct libxl__ev_time {
/* caller should include this in their own struct */
/* read-only for caller, who may read only when registered: */
@@ -734,10 +735,10 @@ static inline void libxl__ev_fd_init(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,
+_hidden int libxl__ev_time_register_rel(libxl__ao*, libxl__ev_time *ev_out,
libxl__ev_time_callback*,
int milliseconds /* as for poll(2) */);
-_hidden int libxl__ev_time_register_abs(libxl__gc*, libxl__ev_time *ev_out,
+_hidden int libxl__ev_time_register_abs(libxl__ao*, libxl__ev_time *ev_out,
libxl__ev_time_callback*,
struct timeval);
_hidden int libxl__ev_time_modify_rel(libxl__gc*, libxl__ev_time *ev,
@@ -1048,13 +1049,13 @@ typedef struct libxl__xswait_state libxl__xswait_state;
* Otherwise, xswait will continue waiting and watching and
* will call you back later.
*
- * rc==ERROR_TIMEDOUT
+ * rc==ERROR_TIMEDOUT, rc==ERROR_CANCELLED
*
* 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, !=ERROR_TIMEDOUT
+ * rc!=0, !=ERROR_TIMEDOUT, !=ERROR_CANCELLED
*
* Some other error occurred.
* This HAS been logged.
@@ -1092,8 +1093,9 @@ int libxl__xswait_start(libxl__gc*, libxl__xswait_state*);
typedef struct libxl__ev_devstate libxl__ev_devstate;
typedef void libxl__ev_devstate_callback(libxl__egc *egc, libxl__ev_devstate*,
int rc);
- /* rc will be 0, ERROR_TIMEDOUT, ERROR_INVAL (meaning path was removed),
- * or ERROR_FAIL if other stuff went wrong (in which latter case, logged) */
+ /* rc will be 0, ERROR_TIMEDOUT, ERROR_CANCELLED, ERROR_INVAL
+ * (meaning path was removed), or ERROR_FAIL if other stuff went
+ * wrong (in which latter case, logged) */
struct libxl__ev_devstate {
/* read-only for caller, who may read only when waiting: */
--
1.7.10.4
next prev parent reply other threads:[~2013-12-20 18:45 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <4B8F5D33B081C044AA43634E84ED7F9616A83D@AMSPEX01CL03.citrite.net>
2013-10-23 17:23 ` FW: Cancelling asynchronous operations in libxl Konrad Rzeszutek Wilk
2013-10-26 8:33 ` Ian Campbell
[not found] ` <1382776392.22417.179.camel@hastur.hellion.org.uk>
2013-10-28 9:38 ` Simon Beaumont
2013-10-28 15:52 ` Ian Jackson
2013-10-31 13:52 ` Ian Campbell
2013-10-31 14:32 ` Ian Jackson
2013-10-31 17:09 ` Ian Campbell
2013-11-08 18:38 ` Ian Jackson
2013-11-20 11:01 ` Ian Campbell
2013-12-20 18:24 ` Ian Jackson
2013-12-20 18:45 ` [RFC PATCH 00/14] libxl: Asynchronous event cancellation Ian Jackson
2013-12-20 18:45 ` [PATCH 01/14] libxl: suspend: switch_logdirty_done takes rc Ian Jackson
2013-12-20 18:45 ` [PATCH 02/14] libxl: suspend: common suspend callbacks take rc Ian Jackson
2013-12-20 18:45 ` [PATCH 03/14] libxl: suspend: Return correct error from callbacks Ian Jackson
2013-12-20 18:45 ` [PATCH 04/14] libxl: Use libxl__xswait* in libxl__ao_device Ian Jackson
2013-12-20 18:45 ` [PATCH 05/14] libxl: xswait/devstate: Move xswait to before devstate Ian Jackson
2013-12-20 18:45 ` [PATCH 06/14] libxl: devstate: Use libxl__xswait* Ian Jackson
2013-12-20 18:45 ` [PATCH 07/14] libxl: New error codes CANCELLED etc Ian Jackson
2013-12-20 18:45 ` Ian Jackson [this message]
2013-12-20 18:45 ` [PATCH 09/14] libxl: domain create: Do not destroy on cancellation Ian Jackson
2013-12-20 18:45 ` [PATCH 10/14] libxl: ao: Record ultimate parent of a nested ao Ian Jackson
2013-12-20 18:45 ` [PATCH 11/14] libxl: ao: Count the nested progeny of an ao Ian Jackson
2013-12-20 18:45 ` [PATCH 12/14] libxl: ao: Provide manip_refcnt Ian Jackson
2013-12-20 18:45 ` [PATCH 13/14] libxl: ao: Cancellation API Ian Jackson
2013-12-20 18:45 ` [PATCH 14/14] libxl: ao: Timeouts are cancellable Ian Jackson
2014-03-14 10:42 ` FW: Cancelling asynchronous operations in libxl Ian Campbell
2014-03-14 12:32 ` Simon Beaumont
2014-03-14 17:09 ` Ian Jackson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1387565152-5642-9-git-send-email-ian.jackson@eu.citrix.com \
--to=ian.jackson@eu.citrix.com \
--cc=ian.campbell@citrix.com \
--cc=xen-devel@lists.xensource.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).