* [PATCH v6 00/20] libxl: subprocess handling
@ 2012-04-13 18:39 Ian Jackson
2012-04-13 18:39 ` [PATCH 01/20] libxl: handle POLLERR, POLLHUP, POLLNVAL properly Ian Jackson
` (19 more replies)
0 siblings, 20 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:39 UTC (permalink / raw)
To: xen-devel
I have not tested the tip of this series. Up to 12/20 have been
tested, in an earlier version.
Changes since v4 include:
- all comments addressed as described in emails on the list
- new patches to convert device model spawning, domain
creation, and console connect callback, to use event model
- possibly, some bugfixes
At the end of this series I think there are no more impermissible
users of fork (or libxl_fork) in libxl. xl stilll uses libxl_fork,
about which something should be done.
Tested and previously-posted (these were 20-31/31 from v4):
01/20 libxl: handle POLLERR, POLLHUP, POLLNVAL properly
02/20 libxl: support multiple libxl__ev_fds for the same fd
03/20 libxl: event API: new facilities for waiting for subprocesses
04/20 autoconf: New test for openpty et al.
05/20 libxl: provide libxl__remove_file et al.
06/20 libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds
07/20 libxl: Clean up setdefault in do_domain_create
08/20 libxl: provide libxl__datacopier_*
09/20 libxl: provide libxl__openpty_*
10/20 libxl: ao: Convert libxl_run_bootloader
11/20 libxl: make libxl_create_logfile const-correct
12/20 libxl: log bootloader output
Preparatory work (new in this version of the series):
13/20 libxl: Allow AO_GC and EGC_GC even if not used
14/20 libxl: remove ctx->waitpid_instead
15/20 libxl: change some structures to unit arrays
19/20 libxl: remove malloc failure handling from NEW_EVENT
Heavy lifting in domain creation (new in this version of the series):
16/20 libxl: ao: convert libxl__spawn_*
17/20 libxl: make libxl_create run bootloader via callback
18/20 libxl: provide progress reporting for long-running operations
20/20 libxl: convert console callback to libxl_asyncprogress_how
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 01/20] libxl: handle POLLERR, POLLHUP, POLLNVAL properly
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
@ 2012-04-13 18:39 ` Ian Jackson
2012-04-16 7:53 ` Ian Campbell
2012-04-13 18:39 ` [PATCH 02/20] libxl: support multiple libxl__ev_fds for the same fd Ian Jackson
` (18 subsequent siblings)
19 siblings, 1 reply; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:39 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Pass POLLERR and POLLHUP to fd callbacks, as is necessary.
Crash on POLLNVAL since that means our fds are messed up.
Document the behaviour (including the fact that poll sometimes sets
POLLHUP or POLLERR even if only POLLIN was requested.
Fix the one current fd callback to do something with POLLERR|POLLHUP.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_event.c | 7 ++++++-
tools/libxl/libxl_internal.h | 5 +++++
2 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 638b9ab..5e1a207 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -335,6 +335,9 @@ static void watchfd_callback(libxl__egc *egc, libxl__ev_fd *ev,
{
EGC_GC;
+ if (revents & (POLLERR|POLLHUP))
+ LIBXL__EVENT_DISASTER(egc, "unexpected poll event on watch fd", 0, 0);
+
for (;;) {
char **event = xs_check_watch(CTX->xsh);
if (!event) {
@@ -739,7 +742,9 @@ static int afterpoll_check_fd(libxl__poller *poller,
/* again, stale slot entry */
return 0;
- int revents = fds[slot].revents & events;
+ assert(!(fds[slot].revents & POLLNVAL));
+
+ int revents = fds[slot].revents & (events | POLLERR | POLLHUP);
/* we mask in case requested events have changed */
return revents;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a4b933b..af631fb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -127,6 +127,11 @@ void libxl__alloc_failed(libxl_ctx *, const char *func,
typedef struct libxl__ev_fd libxl__ev_fd;
typedef void libxl__ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
int fd, short events, short revents);
+ /* Note that revents may contain POLLERR or POLLHUP regardless of
+ * events; otherwise revents contains only bits in events. Contrary
+ * to the documentation for poll(2), POLLERR and POLLHUP can occur
+ * even if only POLLIN was set in events. (POLLNVAL is a fatal
+ * error and will cause libxl event machinery to fail an assertion.) */
struct libxl__ev_fd {
/* caller should include this in their own struct */
/* read-only for caller, who may read only when registered: */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 02/20] libxl: support multiple libxl__ev_fds for the same fd
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
2012-04-13 18:39 ` [PATCH 01/20] libxl: handle POLLERR, POLLHUP, POLLNVAL properly Ian Jackson
@ 2012-04-13 18:39 ` Ian Jackson
2012-04-16 8:04 ` Ian Campbell
2012-04-13 18:39 ` [PATCH 03/20] libxl: event API: new facilities for waiting for subprocesses Ian Jackson
` (17 subsequent siblings)
19 siblings, 1 reply; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:39 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
We need a slightly more sophisticated data structure to allow this,
where we record the slot not just for each fd but also for each
(fd,eventbit) where eventbit is POLLIN, POLLPRI, POLLOUT.
Document the new relaxed restriction.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_event.c | 62 +++++++++++++++++++++++------------------
tools/libxl/libxl_internal.h | 10 +++++--
2 files changed, 42 insertions(+), 30 deletions(-)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 5e1a207..672e3fe 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -635,10 +635,11 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
/*
- * In order to be able to efficiently find the libxl__ev_fd
- * for a struct poll during _afterpoll, we maintain a shadow
- * data structure in CTX->fd_beforepolled: each slot in
- * the fds array corresponds to a slot in fd_beforepolled.
+ * In order to be able to efficiently find the libxl__ev_fd for a
+ * struct poll during _afterpoll, we maintain a shadow data
+ * structure in CTX->fd_rindices: each fd corresponds to a slot in
+ * fd_rindices, and each elemebnt in the rindices is three indices
+ * into the fd array (for POLLIN, POLLPRI and POLLOUT).
*/
if (*nfds_io) {
@@ -659,14 +660,16 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
});
/* make sure our array is as big as *nfds_io */
- if (poller->fd_rindex_allocd < maxfd) {
- assert(maxfd < INT_MAX / sizeof(int) / 2);
- int *newarray = realloc(poller->fd_rindex, sizeof(int) * maxfd);
- if (!newarray) { rc = ERROR_NOMEM; goto out; }
- memset(newarray + poller->fd_rindex_allocd, 0,
- sizeof(int) * (maxfd - poller->fd_rindex_allocd));
- poller->fd_rindex = newarray;
- poller->fd_rindex_allocd = maxfd;
+ if (poller->fd_rindices_allocd < maxfd) {
+ assert(ARRAY_SIZE_OK(poller->fd_rindices, maxfd));
+ poller->fd_rindices =
+ libxl__realloc(0, poller->fd_rindices,
+ maxfd * sizeof(*poller->fd_rindices));
+ memset(poller->fd_rindices + poller->fd_rindices_allocd,
+ 0,
+ (maxfd - poller->fd_rindices_allocd)
+ * sizeof(*poller->fd_rindices));
+ poller->fd_rindices_allocd = maxfd;
}
}
@@ -677,8 +680,10 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
fds[used].fd = req_fd;
fds[used].events = req_events;
fds[used].revents = 0;
- assert(req_fd < poller->fd_rindex_allocd);
- poller->fd_rindex[req_fd] = used;
+ assert(req_fd < poller->fd_rindices_allocd);
+ if (req_events & POLLIN) poller->fd_rindices[req_fd][0] = used;
+ if (req_events & POLLPRI) poller->fd_rindices[req_fd][1] = used;
+ if (req_events & POLLOUT) poller->fd_rindices[req_fd][2] = used;
}
used++;
});
@@ -706,7 +711,6 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
*timeout_upd = our_timeout;
}
- out:
return rc;
}
@@ -728,24 +732,28 @@ static int afterpoll_check_fd(libxl__poller *poller,
int fd, int events)
/* returns mask of events which were requested and occurred */
{
- if (fd >= poller->fd_rindex_allocd)
+ if (fd >= poller->fd_rindices_allocd)
/* added after we went into poll, have to try again */
return 0;
- int slot = poller->fd_rindex[fd];
+ int i, revents = 0;
+ for (i=0; i<3; i++) {
+ int slot = poller->fd_rindices[fd][i];
- if (slot >= nfds)
- /* stale slot entry; again, added afterwards */
- return 0;
+ if (slot >= nfds)
+ /* stale slot entry; again, added afterwards */
+ continue;
- if (fds[slot].fd != fd)
- /* again, stale slot entry */
- return 0;
+ if (fds[slot].fd != fd)
+ /* again, stale slot entry */
+ continue;
- assert(!(fds[slot].revents & POLLNVAL));
+ assert(!(fds[slot].revents & POLLNVAL));
+ revents |= fds[slot].revents;
+ }
- int revents = fds[slot].revents & (events | POLLERR | POLLHUP);
/* we mask in case requested events have changed */
+ revents &= (events | POLLERR | POLLHUP);
return revents;
}
@@ -1009,7 +1017,7 @@ int libxl__poller_init(libxl_ctx *ctx, libxl__poller *p)
{
int r, rc;
p->fd_polls = 0;
- p->fd_rindex = 0;
+ p->fd_rindices = 0;
r = pipe(p->wakeup_pipe);
if (r) {
@@ -1036,7 +1044,7 @@ void libxl__poller_dispose(libxl__poller *p)
if (p->wakeup_pipe[1] > 0) close(p->wakeup_pipe[1]);
if (p->wakeup_pipe[0] > 0) close(p->wakeup_pipe[0]);
free(p->fd_polls);
- free(p->fd_rindex);
+ free(p->fd_rindices);
}
libxl__poller *libxl__poller_get(libxl_ctx *ctx)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index af631fb..2ce4bb5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -131,7 +131,11 @@ typedef void libxl__ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
* events; otherwise revents contains only bits in events. Contrary
* to the documentation for poll(2), POLLERR and POLLHUP can occur
* even if only POLLIN was set in events. (POLLNVAL is a fatal
- * error and will cause libxl event machinery to fail an assertion.) */
+ * error and will cause libxl event machinery to fail an assertion.)
+ *
+ * It is not permitted to listen for the same or overlapping events
+ * on the same fd using multiple different libxl__ev_fd's.
+ */
struct libxl__ev_fd {
/* caller should include this in their own struct */
/* read-only for caller, who may read only when registered: */
@@ -257,8 +261,8 @@ struct libxl__poller {
struct pollfd *fd_polls;
int fd_polls_allocd;
- int fd_rindex_allocd;
- int *fd_rindex; /* see libxl_osevent_beforepoll */
+ int fd_rindices_allocd;
+ int (*fd_rindices)[3]; /* see libxl_osevent_beforepoll */
int wakeup_pipe[2]; /* 0 means no fd allocated */
};
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 03/20] libxl: event API: new facilities for waiting for subprocesses
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
2012-04-13 18:39 ` [PATCH 01/20] libxl: handle POLLERR, POLLHUP, POLLNVAL properly Ian Jackson
2012-04-13 18:39 ` [PATCH 02/20] libxl: support multiple libxl__ev_fds for the same fd Ian Jackson
@ 2012-04-13 18:39 ` Ian Jackson
2012-04-13 18:39 ` [PATCH 04/20] autoconf: New test for openpty et al Ian Jackson
` (16 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:39 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Ian Jackson
The current arrangements in libxl for spawning subprocesses have two
key problems: (i) they make unwarranted (and largely undocumented)
assumptions about the caller's use of subprocesses, (ii) they aren't
integrated into the event system and can't be made asynchronous etc.
So replace them with a new set of facilities.
Primarily, from the point of view of code inside libxl, this is
libxl__ev_child_fork which is both (a) a version of fork() and (b) an
event source which generates a callback when the child dies.
>From the point of view of the application, we fully document our use
of SIGCHLD. The application can tell us whether it wants to own
SIGCHLD or not; if it does, it has to tell us about deaths of our
children.
Currently there are no callers in libxl which use these facilities.
All code in libxl which forks needs to be converted and libxl_fork
needse to be be abolished.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Roger Pau Monne <roger.pau@entel.upc.edu>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 17 +++-
tools/libxl/libxl.h | 1 +
tools/libxl/libxl_event.c | 53 +++++++--
tools/libxl/libxl_event.h | 147 +++++++++++++++++++++++-
tools/libxl/libxl_fork.c | 255 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 62 ++++++++++-
6 files changed, 515 insertions(+), 20 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 60dbfdc..42ac89f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -39,7 +39,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
memset(ctx, 0, sizeof(libxl_ctx));
ctx->lg = lg;
- /* First initialise pointers (cannot fail) */
+ /* First initialise pointers etc. (cannot fail) */
LIBXL_TAILQ_INIT(&ctx->occurred);
@@ -58,6 +58,11 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
LIBXL_TAILQ_INIT(&ctx->death_list);
libxl__ev_xswatch_init(&ctx->death_watch);
+ ctx->childproc_hooks = &libxl__childproc_default_hooks;
+ ctx->childproc_user = 0;
+
+ ctx->sigchld_selfpipe[0] = -1;
+
/* The mutex is special because we can't idempotently destroy it */
if (libxl__init_recursive_mutex(ctx, &ctx->lock) < 0) {
@@ -160,6 +165,16 @@ int libxl_ctx_free(libxl_ctx *ctx)
discard_events(&ctx->occurred);
+ /* If we have outstanding children, then the application inherits
+ * them; we wish the application good luck with understanding
+ * this if and when it reaps them. */
+ libxl__sigchld_removehandler(ctx);
+
+ if (ctx->sigchld_selfpipe[0] >= 0) {
+ close(ctx->sigchld_selfpipe[0]);
+ close(ctx->sigchld_selfpipe[1]);
+ }
+
pthread_mutex_destroy(&ctx->lock);
GC_FREE;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index edbca53..03e71f6 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -380,6 +380,7 @@ enum {
ERROR_NOT_READY = -11,
ERROR_OSEVENT_REG_FAIL = -12,
ERROR_BUFFERFULL = -13,
+ ERROR_UNKNOWN_CHILD = -14,
};
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 672e3fe..1b7495a 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -623,6 +623,10 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
\
REQUIRE_FD(poller->wakeup_pipe[0], POLLIN, BODY); \
\
+ int selfpipe = libxl__fork_selfpipe_active(CTX); \
+ if (selfpipe >= 0) \
+ REQUIRE_FD(selfpipe, POLLIN, BODY); \
+ \
}while(0)
#define REQUIRE_FD(req_fd_, req_events_, BODY) do{ \
@@ -762,10 +766,11 @@ static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
int nfds, const struct pollfd *fds,
struct timeval now)
{
+ /* May make callbacks into the application for child processes.
+ * ctx must be locked exactly once */
EGC_GC;
libxl__ev_fd *efd;
-
LIBXL_LIST_FOREACH(efd, &CTX->efds, entry) {
if (!efd->events)
continue;
@@ -776,11 +781,16 @@ static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
}
if (afterpoll_check_fd(poller,fds,nfds, poller->wakeup_pipe[0],POLLIN)) {
- char buf[256];
- int r = read(poller->wakeup_pipe[0], buf, sizeof(buf));
- if (r < 0)
- if (errno != EINTR && errno != EWOULDBLOCK)
- LIBXL__EVENT_DISASTER(egc, "read wakeup", errno, 0);
+ int e = libxl__self_pipe_eatall(poller->wakeup_pipe[0]);
+ if (e) LIBXL__EVENT_DISASTER(egc, "read wakeup", e, 0);
+ }
+
+ int selfpipe = libxl__fork_selfpipe_active(CTX);
+ if (selfpipe >= 0 &&
+ afterpoll_check_fd(poller,fds,nfds, selfpipe, POLLIN)) {
+ int e = libxl__self_pipe_eatall(selfpipe);
+ if (e) LIBXL__EVENT_DISASTER(egc, "read sigchld pipe", e, 0);
+ libxl__fork_selfpipe_woken(egc);
}
for (;;) {
@@ -1078,16 +1088,37 @@ void libxl__poller_put(libxl_ctx *ctx, libxl__poller *p)
void libxl__poller_wakeup(libxl__egc *egc, libxl__poller *p)
{
+ int e = libxl__self_pipe_wakeup(p->wakeup_pipe[1]);
+ if (e) LIBXL__EVENT_DISASTER(egc, "cannot poke watch pipe", e, 0);
+}
+
+int libxl__self_pipe_wakeup(int fd)
+{
static const char buf[1] = "";
for (;;) {
- int r = write(p->wakeup_pipe[1], buf, 1);
- if (r==1) return;
+ int r = write(fd, buf, 1);
+ if (r==1) return 0;
assert(r==-1);
if (errno == EINTR) continue;
- if (errno == EWOULDBLOCK) return;
- LIBXL__EVENT_DISASTER(egc, "cannot poke watch pipe", errno, 0);
- return;
+ if (errno == EWOULDBLOCK) return 0;
+ assert(errno);
+ return errno;
+ }
+}
+
+int libxl__self_pipe_eatall(int fd)
+{
+ char buf[256];
+ for (;;) {
+ int r = read(fd, buf, sizeof(buf));
+ if (r == sizeof(buf)) continue;
+ if (r >= 0) return 0;
+ assert(r == -1);
+ if (errno == EINTR) continue;
+ if (errno == EWOULDBLOCK) return 0;
+ assert(errno);
+ return errno;
}
}
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index 2d2196f..713d96d 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -163,11 +163,6 @@ void libxl_event_register_callbacks(libxl_ctx *ctx,
* After libxl_ctx_free, all corresponding evgen handles become
* invalid and must no longer be passed to evdisable.
*
- * Events enabled with evenable prior to a fork and libxl_ctx_postfork
- * are no longer generated after the fork/postfork; however the evgen
- * structures are still valid and must be passed to evdisable if the
- * memory they use should not be leaked.
- *
* Applications should ensure that they eventually retrieve every
* event using libxl_event_check or libxl_event_wait, since events
* which occur but are not retreived by the application will be queued
@@ -372,6 +367,148 @@ void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl);
+/*======================================================================*/
+
+/*
+ * Subprocess handling.
+ *
+ * Unfortunately the POSIX interface makes this very awkward.
+ *
+ * There are two possible arrangements for collecting statuses from
+ * wait/waitpid.
+ *
+ * For naive programs:
+ *
+ * libxl will keep a SIGCHLD handler installed whenever it has an
+ * active (unreaped) child. It will reap all children with
+ * wait(); any children it does not recognise will be passed to
+ * the application via an optional callback (and will result in
+ * logged warnings if no callback is provided or the callback
+ * denies responsibility for the child).
+ *
+ * libxl may have children whenever:
+ *
+ * - libxl is performing an operation which can be made
+ * asynchronous; ie one taking a libxl_asyncop_how, even
+ * if NULL is passed indicating that the operation is
+ * synchronous; or
+ *
+ * - events of any kind are being generated, as requested
+ * by libxl_evenable_....
+ *
+ * A multithreaded application which is naive in this sense may
+ * block SIGCHLD on some of its threads, but there must be at
+ * least one thread that has SIGCHLD unblocked. libxl will not
+ * modify the blocking flag for SIGCHLD (except that it may create
+ * internal service threads with all signals blocked).
+ *
+ * A naive program must only have at any one time only
+ * one libxl context which might have children.
+ *
+ * For programs which run their own children alongside libxl's:
+ *
+ * A program which does this must call libxl_childproc_setmode.
+ * There are two options:
+ *
+ * libxl_sigchld_owner_mainloop:
+ * The application must install a SIGCHLD handler and reap (at
+ * least) all of libxl's children and pass their exit status
+ * to libxl by calling libxl_childproc_exited.
+ *
+ * libxl_sigchld_owner_libxl_always:
+ * The application expects libxl to reap all of its children,
+ * and provides a callback to be notified of their exit
+ * statues.
+ *
+ * An application which fails to call setmode, or which passes 0 for
+ * hooks, while it uses any libxl operation which might
+ * create or use child processes (see above):
+ * - Must not have any child processes running.
+ * - Must not install a SIGCHLD handler.
+ * - Must not reap any children.
+ */
+
+
+typedef enum {
+ /* libxl owns SIGCHLD whenever it has a child. */
+ libxl_sigchld_owner_libxl,
+
+ /* Application promises to call libxl_childproc_exited but NOT
+ * from within a signal handler. libxl will not itself arrange to
+ * (un)block or catch SIGCHLD. */
+ libxl_sigchld_owner_mainloop,
+
+ /* libxl owns SIGCHLD all the time, and the application is
+ * relying on libxl's event loop for reaping its own children. */
+ libxl_sigchld_owner_libxl_always,
+} libxl_sigchld_owner;
+
+typedef struct {
+ libxl_sigchld_owner chldowner;
+
+ /* All of these are optional: */
+
+ /* Called by libxl instead of fork. Should behave exactly like
+ * fork, including setting errno etc. May NOT reenter into libxl.
+ * Application may use this to discover pids of libxl's children,
+ * for example.
+ */
+ pid_t (*fork_replacement)(void *user);
+
+ /* With libxl_sigchld_owner_libxl, called by libxl when it has
+ * reaped a pid. (Not permitted with _owner_mainloop.)
+ *
+ * Should return 0 if the child was recognised by the application
+ * (or if the application does not keep those kind of records),
+ * ERROR_UNKNOWN_CHILD if the application knows that the child is not
+ * the application's; if it returns another error code it is a
+ * disaster as described for libxl_event_register_callbacks.
+ * (libxl will report unexpected children to its error log.)
+ *
+ * If not supplied, the application is assumed not to start
+ * any children of its own.
+ *
+ * This function is NOT called from within the signal handler.
+ * Rather it will be called from inside a libxl's event handling
+ * code and thus only when libxl is running, for example from
+ * within libxl_event_wait. (libxl uses the self-pipe trick
+ * to implement this.)
+ *
+ * childproc_exited_callback may call back into libxl, but it
+ * is best to avoid making long-running libxl calls as that might
+ * stall the calling event loop while the nested operation
+ * completes.
+ */
+ int (*reaped_callback)(pid_t, int status, void *user);
+} libxl_childproc_hooks;
+
+/* hooks may be 0 in which is equivalent to &{ libxl_sigchld_owner_libxl, 0, 0 }
+ *
+ * May not be called when libxl might have any child processes, or the
+ * behaviour is undefined. So it is best to call this at
+ * initialisation.
+ */
+void libxl_childproc_setmode(libxl_ctx *ctx, const libxl_childproc_hooks *hooks,
+ void *user);
+
+/*
+ * This function is for an application which owns SIGCHLD and which
+ * therefore reaps all of the process's children.
+ *
+ * May be called only by an application which has called setmode with
+ * chldowner == libxl_sigchld_owner_mainloop. If pid was a process started
+ * by this instance of libxl, returns 0 after doing whatever
+ * processing is appropriate. Otherwise silently returns
+ * ERROR_UNKNOWN_CHILD. No other error returns are possible.
+ *
+ * May NOT be called from within a signal handler which might
+ * interrupt any libxl operation. The application will almost
+ * certainly need to use the self-pipe trick (or a working pselect or
+ * ppoll) to implement this.
+ */
+int libxl_childproc_reaped(libxl_ctx *ctx, pid_t, int status);
+
+
/*
* An application which initialises a libxl_ctx in a parent process
* and then forks a child which does not quickly exec, must
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index dce88ad..35c8bdd 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -46,6 +46,12 @@ static int atfork_registered;
static LIBXL_LIST_HEAD(, libxl__carefd) carefds =
LIBXL_LIST_HEAD_INITIALIZER(carefds);
+/* non-null iff installed, protected by no_forking */
+static libxl_ctx *sigchld_owner;
+static struct sigaction sigchld_saved_action;
+
+static void sigchld_removehandler_core(void);
+
static void atfork_lock(void)
{
int r = pthread_mutex_lock(&no_forking);
@@ -107,6 +113,7 @@ void libxl_postfork_child_noexec(libxl_ctx *ctx)
int r;
atfork_lock();
+
LIBXL_LIST_FOREACH_SAFE(cf, &carefds, entry, cf_tmp) {
if (cf->fd >= 0) {
r = close(cf->fd);
@@ -118,6 +125,10 @@ void libxl_postfork_child_noexec(libxl_ctx *ctx)
free(cf);
}
LIBXL_LIST_INIT(&carefds);
+
+ if (sigchld_owner)
+ sigchld_removehandler_core();
+
atfork_unlock();
}
@@ -141,6 +152,250 @@ int libxl__carefd_fd(const libxl__carefd *cf)
}
/*
+ * Actual child process handling
+ */
+
+static void sigchld_handler(int signo)
+{
+ int e = libxl__self_pipe_wakeup(sigchld_owner->sigchld_selfpipe[1]);
+ assert(!e); /* errors are probably EBADF, very bad */
+}
+
+static void sigchld_removehandler_core(void)
+{
+ struct sigaction was;
+ int r;
+
+ r = sigaction(SIGCHLD, &sigchld_saved_action, &was);
+ assert(!r);
+ assert(!(was.sa_flags & SA_SIGINFO));
+ assert(was.sa_handler == sigchld_handler);
+ sigchld_owner = 0;
+}
+
+void libxl__sigchld_removehandler(libxl_ctx *ctx) /* non-reentrant */
+{
+ atfork_lock();
+ if (sigchld_owner == ctx)
+ sigchld_removehandler_core();
+ atfork_unlock();
+}
+
+int libxl__sigchld_installhandler(libxl_ctx *ctx) /* non-reentrant */
+{
+ int r, rc;
+
+ if (ctx->sigchld_selfpipe[0] < 0) {
+ r = pipe(ctx->sigchld_selfpipe);
+ if (r) {
+ ctx->sigchld_selfpipe[0] = -1;
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+ "failed to create sigchld pipe");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+ atfork_lock();
+ if (sigchld_owner != ctx) {
+ struct sigaction ours;
+
+ assert(!sigchld_owner);
+ sigchld_owner = ctx;
+
+ memset(&ours,0,sizeof(ours));
+ ours.sa_handler = sigchld_handler;
+ sigemptyset(&ours.sa_mask);
+ ours.sa_flags = SA_NOCLDSTOP | SA_RESTART;
+ r = sigaction(SIGCHLD, &ours, &sigchld_saved_action);
+ assert(!r);
+
+ assert(((void)"application must negotiate with libxl about SIGCHLD",
+ !(sigchld_saved_action.sa_flags & SA_SIGINFO) &&
+ (sigchld_saved_action.sa_handler == SIG_DFL ||
+ sigchld_saved_action.sa_handler == SIG_IGN)));
+ }
+ atfork_unlock();
+
+ rc = 0;
+ out:
+ return rc;
+}
+
+static int chldmode_ours(libxl_ctx *ctx)
+{
+ return ctx->childproc_hooks->chldowner == libxl_sigchld_owner_libxl;
+}
+
+int libxl__fork_selfpipe_active(libxl_ctx *ctx)
+{
+ /* Returns the fd to read, or -1 */
+ if (!chldmode_ours(ctx))
+ return -1;
+
+ if (LIBXL_LIST_EMPTY(&ctx->children))
+ return -1;
+
+ return ctx->sigchld_selfpipe[0];
+}
+
+static void perhaps_removehandler(libxl_ctx *ctx)
+{
+ if (LIBXL_LIST_EMPTY(&ctx->children) &&
+ ctx->childproc_hooks->chldowner != libxl_sigchld_owner_libxl_always)
+ libxl__sigchld_removehandler(ctx);
+}
+
+static int childproc_reaped(libxl__egc *egc, pid_t pid, int status)
+{
+ EGC_GC;
+ libxl__ev_child *ch;
+
+ LIBXL_LIST_FOREACH(ch, &CTX->children, entry)
+ if (ch->pid == pid)
+ goto found;
+
+ /* not found */
+ return ERROR_UNKNOWN_CHILD;
+
+ found:
+ LIBXL_LIST_REMOVE(ch, entry);
+ ch->pid = -1;
+ ch->callback(egc, ch, pid, status);
+
+ perhaps_removehandler(CTX);
+
+ return 0;
+}
+
+int libxl_childproc_reaped(libxl_ctx *ctx, pid_t pid, int status)
+{
+ EGC_INIT(ctx);
+ CTX_LOCK;
+ int rc = childproc_reaped(egc, pid, status);
+ CTX_UNLOCK;
+ EGC_FREE;
+ return rc;
+}
+
+void libxl__fork_selfpipe_woken(libxl__egc *egc)
+{
+ /* May make callbacks into the application for child processes.
+ * ctx must be locked EXACTLY ONCE */
+ EGC_GC;
+
+ while (chldmode_ours(CTX) /* in case the app changes the mode */) {
+ int status;
+ pid_t pid = waitpid(-1, &status, WNOHANG);
+
+ if (pid == 0) return;
+
+ if (pid == -1) {
+ if (errno == ECHILD) return;
+ if (errno == EINTR) continue;
+ LIBXL__EVENT_DISASTER(egc, "waitpid() failed", errno, 0);
+ return;
+ }
+
+ int rc = childproc_reaped(egc, pid, status);
+
+ if (rc) {
+ if (CTX->childproc_hooks->reaped_callback) {
+ CTX_UNLOCK;
+ rc = CTX->childproc_hooks->reaped_callback
+ (pid, status, CTX->childproc_user);
+ CTX_LOCK;
+ if (rc != 0 && rc != ERROR_UNKNOWN_CHILD) {
+ char disasterbuf[200];
+ snprintf(disasterbuf, sizeof(disasterbuf), " reported by"
+ " libxl_childproc_hooks->reaped_callback"
+ " (for pid=%lu, status=%d; error code %d)",
+ (unsigned long)pid, status, rc);
+ LIBXL__EVENT_DISASTER(egc, disasterbuf, 0, 0);
+ return;
+ }
+ } else {
+ rc = ERROR_UNKNOWN_CHILD;
+ }
+ if (rc)
+ libxl_report_child_exitstatus(CTX, XTL_WARN,
+ "unknown child", (long)pid, status);
+ }
+ }
+}
+
+pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child *ch,
+ libxl__ev_child_callback *death)
+{
+ CTX_LOCK;
+ int rc;
+
+ if (chldmode_ours(CTX)) {
+ rc = libxl__sigchld_installhandler(CTX);
+ if (rc) goto out;
+ }
+
+ pid_t pid =
+ CTX->childproc_hooks->fork_replacement
+ ? CTX->childproc_hooks->fork_replacement(CTX->childproc_user)
+ : fork();
+ if (pid == -1) {
+ LOGE(ERROR, "fork failed");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (!pid) {
+ /* woohoo! */
+ return 0; /* Yes, CTX is left locked in the child. */
+ }
+
+ ch->pid = pid;
+ ch->callback = death;
+ LIBXL_LIST_INSERT_HEAD(&CTX->children, ch, entry);
+ rc = pid;
+
+ out:
+ perhaps_removehandler(CTX);
+ CTX_UNLOCK;
+ return rc;
+}
+
+void libxl_childproc_setmode(libxl_ctx *ctx, const libxl_childproc_hooks *hooks,
+ void *user)
+{
+ GC_INIT(ctx);
+ CTX_LOCK;
+
+ assert(LIBXL_LIST_EMPTY(&CTX->children));
+
+ if (!hooks)
+ hooks = &libxl__childproc_default_hooks;
+
+ ctx->childproc_hooks = hooks;
+ ctx->childproc_user = user;
+
+ switch (ctx->childproc_hooks->chldowner) {
+ case libxl_sigchld_owner_mainloop:
+ case libxl_sigchld_owner_libxl:
+ libxl__sigchld_removehandler(ctx);
+ break;
+ case libxl_sigchld_owner_libxl_always:
+ libxl__sigchld_installhandler(ctx);
+ break;
+ default:
+ abort();
+ }
+
+ CTX_UNLOCK;
+ GC_FREE;
+}
+
+const libxl_childproc_hooks libxl__childproc_default_hooks = {
+ libxl_sigchld_owner_libxl, 0, 0
+};
+
+/*
* Local variables:
* mode: C
* c-basic-offset: 4
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2ce4bb5..abc38fb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -196,6 +196,19 @@ typedef struct libxl__ev_watch_slot {
libxl__ev_xswatch *libxl__watch_slot_contents(libxl__gc *gc, int slotnum);
+typedef struct libxl__ev_child libxl__ev_child;
+typedef void libxl__ev_child_callback(libxl__egc *egc, libxl__ev_child*,
+ pid_t pid, int status);
+struct libxl__ev_child {
+ /* caller should include this in their own struct */
+ /* read-only for caller: */
+ pid_t pid; /* -1 means unused ("unregistered", ie Idle) */
+ libxl__ev_child_callback *callback;
+ /* remainder is private for libxl__ev_... */
+ LIBXL_LIST_ENTRY(struct libxl__ev_child) entry;
+};
+
+
/*
* evgen structures, which are the state we use for generating
* events for the caller.
@@ -315,10 +328,14 @@ struct libxl__ctx {
LIBXL_LIST_HEAD(, libxl_evgen_disk_eject) disk_eject_evgens;
- /* for callers who reap children willy-nilly; caller must only
- * set this after libxl_init and before any other call - or
- * may leave them untouched */
+ const libxl_childproc_hooks *childproc_hooks;
+ void *childproc_user;
+ int sigchld_selfpipe[2]; /* [0]==-1 means handler not installed */
+ LIBXL_LIST_HEAD(, libxl__ev_child) children;
+
+ /* This is obsolete and must be removed: */
int (*waitpid_instead)(pid_t pid, int *status, int flags);
+
libxl_version_info version_info;
};
@@ -566,6 +583,36 @@ static inline int libxl__ev_xswatch_isregistered(const libxl__ev_xswatch *xw)
/*
+ * For making subprocesses. This is the only permitted mechanism for
+ * code in libxl to do so.
+ *
+ * In the parent, returns the pid, filling in childw_out.
+ * In the child, returns 0.
+ * If it fails, returns a libxl error (all of which are -ve).
+ *
+ * The child should go on to exec (or exit) soon. The child may not
+ * make any further calls to libxl infrastructure, except for memory
+ * allocation and logging. If the child needs to use xenstore it
+ * must open its own xs handle and use it directly, rather than via
+ * the libxl event machinery.
+ *
+ * The parent may signal the child but it must not reap it. That will
+ * be done by the event machinery. death may be NULL in which case
+ * the child is still reaped but its death is ignored.
+ *
+ * It is not possible to "deregister" the child death event source.
+ * It will generate exactly one event callback; until then the childw
+ * is Active and may not be reused.
+ */
+_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)
+ { return childw_out->pid >= 0; }
+
+
+/*
* Other event-handling support provided by the libxl event core to
* the rest of libxl.
*/
@@ -619,6 +666,15 @@ void libxl__poller_put(libxl_ctx *ctx, libxl__poller *p);
* ctx must be locked. */
void libxl__poller_wakeup(libxl__egc *egc, libxl__poller *p);
+/* Internal to fork and child reaping machinery */
+extern const libxl_childproc_hooks libxl__childproc_default_hooks;
+int libxl__sigchld_installhandler(libxl_ctx *ctx); /* non-reentrant;logs errs */
+void libxl__sigchld_removehandler(libxl_ctx *ctx); /* non-reentrant */
+int libxl__fork_selfpipe_active(libxl_ctx *ctx); /* returns read fd or -1 */
+void libxl__fork_selfpipe_woken(libxl__egc *egc);
+int libxl__self_pipe_wakeup(int fd); /* returns 0 or -1 setting errno */
+int libxl__self_pipe_eatall(int fd); /* returns 0 or -1 setting errno */
+
int libxl__atfork_init(libxl_ctx *ctx);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 04/20] autoconf: New test for openpty et al.
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (2 preceding siblings ...)
2012-04-13 18:39 ` [PATCH 03/20] libxl: event API: new facilities for waiting for subprocesses Ian Jackson
@ 2012-04-13 18:39 ` Ian Jackson
2012-04-16 9:09 ` Ian Campbell
2012-04-16 10:09 ` Roger Pau Monne
2012-04-13 18:39 ` [PATCH 05/20] libxl: provide libxl__remove_file " Ian Jackson
` (15 subsequent siblings)
19 siblings, 2 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:39 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
We may need to #include <libutil.h>, and/or link with -lutil, to use
openpty, login_tty, and the like. Provide INCLUDE_LIBUTIL_H
(preprocessor constant, not always defined) and PTYFUNCS_LIBS
(makefile variable).
We link libxl against PTYFUNCS_LIBS (which comes from autoconf) rather
than UTIL_LIBS, and #include <libutil.h> where appropriate.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
config/Tools.mk.in | 2 +
tools/configure | 51 ++++++++++++++++++++++++++++++++++++++++
tools/configure.ac | 2 +
tools/libxl/Makefile | 2 +-
tools/libxl/libxl_bootloader.c | 4 +++
tools/m4/ptyfuncs.m4 | 24 ++++++++++++++++++
6 files changed, 84 insertions(+), 1 deletions(-)
create mode 100644 tools/m4/ptyfuncs.m4
diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 912d021..eb879dd 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -30,6 +30,8 @@ PTHREAD_CFLAGS := @PTHREAD_CFLAGS@
PTHREAD_LDFLAGS := @PTHREAD_LDFLAGS@
PTHREAD_LIBS := @PTHREAD_LIBS@
+PTYFUNCS_LIBS := @PTYFUNCS_LIBS@
+
# Download GIT repositories via HTTP or GIT's own protocol?
# GIT's protocol is faster and more robust, when it works at all (firewalls
# may block it). We make it the default, but if your GIT repository downloads
diff --git a/tools/configure b/tools/configure
index 86618f5..eeaf901 100755
--- a/tools/configure
+++ b/tools/configure
@@ -602,6 +602,7 @@ POW_LIB
LIBOBJS
ALLOCA
libiconv
+PTYFUNCS_LIBS
PTHREAD_LIBS
PTHREAD_LDFLAGS
PTHREAD_CFLAGS
@@ -3947,6 +3948,8 @@ case $host_os in *\ *) host_os=`echo "$host_os" | sed 's/ /-/g'`;; esac
+
+
# Enable/disable options
# Check whether --enable-githttp was given.
@@ -7315,6 +7318,54 @@ $as_echo "$ax_cv_pthread_flags" >&6; }
+
+ ac_fn_c_check_header_mongrel "$LINENO" "libutil.h" "ac_cv_header_libutil_h" "$ac_includes_default"
+if test "x$ac_cv_header_libutil_h" = x""yes; then :
+
+
+$as_echo "#define INCLUDE_LIBUTIL_H <libutil.h>" >>confdefs.h
+
+
+fi
+
+
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for openpty et al" >&5
+$as_echo_n "checking for openpty et al... " >&6; }
+if test "${ax_cv_ptyfuncs_libs+set}" = set; then :
+ $as_echo_n "(cached) " >&6
+else
+
+ ax_cv_ptyfuncs_libs=-lutil
+
+ saved_LIBS="$LIBS"
+
+ LIBS="$LIBS $ax_cv_ptyfuncs_libs"
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+#ifdef INCLUDE_LIBUTIL_H
+#include INCLUDE_LIBUTIL_H
+#endif
+int main(void) {
+ openpty(0,0,0,0,0);
+ login_tty(0);
+}
+
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+
+fi
+rm -f core conftest.err conftest.$ac_objext \
+ conftest$ac_exeext conftest.$ac_ext
+
+ LIBS="$saved_LIBS"
+
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ax_cv_ptyfuncs_libs" >&5
+$as_echo "$ax_cv_ptyfuncs_libs" >&6; }
+ PTYFUNCS_LIBS="$ax_cv_ptyfuncs_libs"
+
+
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for clock_gettime in -lrt" >&5
$as_echo_n "checking for clock_gettime in -lrt... " >&6; }
if test "${ac_cv_lib_rt_clock_gettime+set}" = set; then :
diff --git a/tools/configure.ac b/tools/configure.ac
index 250dffd..b4f72e8 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -35,6 +35,7 @@ m4_include([m4/uuid.m4])
m4_include([m4/pkg.m4])
m4_include([m4/curses.m4])
m4_include([m4/pthread.m4])
+m4_include([m4/ptyfuncs.m4])
# Enable/disable options
AX_ARG_DEFAULT_DISABLE([githttp], [Download GIT repositories via HTTP])
@@ -132,6 +133,7 @@ AC_SUBST(libext2fs)
AC_CHECK_LIB([gcrypt], [gcry_md_hash_buffer], [libgcrypt="y"], [libgcrypt="n"])
AC_SUBST(libgcrypt)
AX_CHECK_PTHREAD
+AX_CHECK_PTYFUNCS
AC_CHECK_LIB([rt], [clock_gettime])
AC_CHECK_LIB([yajl], [yajl_alloc], [],
[AC_MSG_ERROR([Could not find yajl])])
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index e5ea867..5ba144f 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid
endif
LIBXL_LIBS =
-LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(UTIL_LIBS) $(LIBUUID_LIBS)
+LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
CFLAGS += $(PTHREAD_CFLAGS)
LDFLAGS += $(PTHREAD_LDFLAGS)
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 2774062..b50944a 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -16,6 +16,10 @@
#include <termios.h>
+#ifdef INCLUDE_LIBUTIL_H
+#include INCLUDE_LIBUTIL_H
+#endif
+
#include "libxl_internal.h"
#define XENCONSOLED_BUF_SIZE 16
diff --git a/tools/m4/ptyfuncs.m4 b/tools/m4/ptyfuncs.m4
new file mode 100644
index 0000000..2846a6d
--- /dev/null
+++ b/tools/m4/ptyfuncs.m4
@@ -0,0 +1,24 @@
+AC_DEFUN([AX_CHECK_PTYFUNCS], [
+ AC_CHECK_HEADER([libutil.h],[
+ AC_DEFINE([INCLUDE_LIBUTIL_H],[<libutil.h>],[libutil header file name])
+ ])
+ AC_CACHE_CHECK([for openpty et al], [ax_cv_ptyfuncs_libs], [
+ ax_cv_ptyfuncs_libs=-lutil
+ AX_SAVEVAR_SAVE(LIBS)
+ LIBS="$LIBS $ax_cv_ptyfuncs_libs"
+ AC_LINK_IFELSE([
+#ifdef INCLUDE_LIBUTIL_H
+#include INCLUDE_LIBUTIL_H
+#endif
+int main(void) {
+ openpty(0,0,0,0,0);
+ login_tty(0);
+}
+])
+ AX_SAVEVAR_RESTORE(LIBS)],
+ [],[
+ AC_MSG_FAILURE([Unable to find -lutil for openpty and login_tty])
+ ])
+ PTYFUNCS_LIBS="$ax_cv_ptyfuncs_libs"
+ AC_SUBST(PTYFUNCS_LIBS)
+])
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 05/20] libxl: provide libxl__remove_file et al.
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (3 preceding siblings ...)
2012-04-13 18:39 ` [PATCH 04/20] autoconf: New test for openpty et al Ian Jackson
@ 2012-04-13 18:39 ` Ian Jackson
2012-04-16 9:24 ` Ian Campbell
2012-04-13 18:40 ` [PATCH 06/20] libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds Ian Jackson
` (14 subsequent siblings)
19 siblings, 1 reply; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:39 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
These utility functions cope with EINTR AND ENOENT, do error logging,
and we provide a recursive version to delete whole directory trees.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_internal.h | 7 ++++
tools/libxl/libxl_utils.c | 79 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index abc38fb..3996824 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -442,6 +442,13 @@ _hidden char *libxl__strndup(libxl__gc *gc_opt, const char *c, size_t n);
* string. (similar to a gc'd dirname(3)). */
_hidden char *libxl__dirname(libxl__gc *gc_opt, const char *s);
+/* Each of these logs errors and returns a libxl error code.
+ * They do not mind if path is already removed.
+ * For _file, path must not be a directory; for _directory it must be. */
+_hidden int libxl__remove_file(libxl__gc *gc, const char *path);
+_hidden int libxl__remove_directory(libxl__gc *gc, const char *path);
+_hidden int libxl__remove_file_or_directory(libxl__gc *gc, const char *path);
+
_hidden char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array, int length);
_hidden int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 0cbd85e..1a4874c 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -364,6 +364,85 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
READ_WRITE_EXACTLY(read, 1, /* */)
READ_WRITE_EXACTLY(write, 0, const)
+int libxl__remove_file(libxl__gc *gc, const char *path)
+{
+ for (;;) {
+ int r = unlink(path);
+ if (!r) return 0;
+ if (errno == ENOENT) return 0;
+ if (errno == EINTR) continue;
+ LOGE(ERROR, "failed to remove file %s", path);
+ return ERROR_FAIL;
+ }
+}
+
+int libxl__remove_file_or_directory(libxl__gc *gc, const char *path)
+{
+ for (;;) {
+ int r = rmdir(path);
+ if (!r) return 0;
+ if (errno == ENOENT) return 0;
+ if (errno == ENOTEMPTY) return libxl__remove_directory(gc, path);
+ if (errno == ENOTDIR) return libxl__remove_file(gc, path);
+ if (errno == EINTR) continue;
+ LOGE(ERROR, "failed to remove %s", path);
+ return ERROR_FAIL;
+ }
+}
+
+int libxl__remove_directory(libxl__gc *gc, const char *dirpath)
+{
+ int rc = 0;
+ DIR *d = 0;
+
+ d = opendir(dirpath);
+ if (!d) {
+ if (errno == ENOENT)
+ goto out;
+
+ LOGE(ERROR, "failed to opendir %s for removal", dirpath);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ size_t need = offsetof(struct dirent, d_name) +
+ pathconf(dirpath, _PC_NAME_MAX) + 1;
+ struct dirent *de_buf = libxl__zalloc(gc, need);
+ struct dirent *de;
+
+ for (;;) {
+ int r = readdir_r(d, de_buf, &de);
+ if (r) {
+ LOGE(ERROR, "failed to readdir %s for removal", dirpath);
+ rc = ERROR_FAIL;
+ break;
+ }
+ if (!de)
+ break;
+
+ if (!strcmp(de->d_name, ".") ||
+ !strcmp(de->d_name, ".."))
+ continue;
+
+ const char *subpath = GCSPRINTF("%s/%s", dirpath, de->d_name);
+ if (libxl__remove_file_or_directory(gc, subpath))
+ rc = ERROR_FAIL;
+ }
+
+ for (;;) {
+ int r = rmdir(dirpath);
+ if (!r) break;
+ if (errno == ENOENT) goto out;
+ if (errno == EINTR) continue;
+ LOGE(ERROR, "failed to remove emptied directory %s", dirpath);
+ rc = ERROR_FAIL;
+ }
+
+ out:
+ if (d) closedir(d);
+
+ return rc;
+}
pid_t libxl_fork(libxl_ctx *ctx)
{
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 06/20] libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (4 preceding siblings ...)
2012-04-13 18:39 ` [PATCH 05/20] libxl: provide libxl__remove_file " Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 07/20] libxl: Clean up setdefault in do_domain_create Ian Jackson
` (13 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
We will want to reuse the fd-sending code, so break it out into its
own function, and provide the corresponding sending function.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_internal.h | 11 ++++
tools/libxl/libxl_qmp.c | 31 ++-----------
tools/libxl/libxl_utils.c | 104 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 119 insertions(+), 27 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3996824..6ceb362 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1128,6 +1128,17 @@ _hidden void libxl__qmp_cleanup(libxl__gc *gc, uint32_t domid);
_hidden int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
const libxl_domain_config *guest_config);
+/* on failure, logs */
+int libxl__sendmsg_fds(libxl__gc *gc, int carrier,
+ const void *data, size_t datalen,
+ int nfds, const int fds[], const char *what);
+
+/* Insists on receiving exactly nfds and datalen. On failure, logs
+ * and leaves *fds untouched. */
+int libxl__recvmsg_fds(libxl__gc *gc, int carrier,
+ void *databuf, size_t datalen,
+ int nfds, int fds[], const char *what);
+
/* from libxl_json */
#include <yajl/yajl_gen.h>
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index f5a3edc..83c22b3 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -544,38 +544,15 @@ static int qmp_send_fd(libxl__gc *gc, libxl__qmp_handler *qmp,
qmp_request_context *context,
int fd)
{
- struct msghdr msg = { 0 };
- struct cmsghdr *cmsg;
- char control[CMSG_SPACE(sizeof (fd))];
- struct iovec iov;
char *buf = NULL;
+ int rc;
buf = qmp_send_prepare(gc, qmp, "getfd", args, callback, opaque, context);
- /* Response data */
- iov.iov_base = buf;
- iov.iov_len = strlen(buf);
-
- /* compose the message */
- msg.msg_iov = &iov;
- msg.msg_iovlen = 1;
- msg.msg_control = control;
- msg.msg_controllen = sizeof (control);
-
- /* attach open fd */
- cmsg = CMSG_FIRSTHDR(&msg);
- cmsg->cmsg_level = SOL_SOCKET;
- cmsg->cmsg_type = SCM_RIGHTS;
- cmsg->cmsg_len = CMSG_LEN(sizeof (fd));
- *(int *)CMSG_DATA(cmsg) = fd;
-
- msg.msg_controllen = cmsg->cmsg_len;
+ rc = libxl__sendmsg_fds(gc, qmp->qmp_fd, buf, strlen(buf), 1, &fd,
+ "QMP message to QEMU");
+ if (rc) return rc;
- if (sendmsg(qmp->qmp_fd, &msg, 0) < 0) {
- LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR,
- "Failed to send a QMP message to QEMU.");
- return ERROR_FAIL;
- }
if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, "\r\n", 2,
"CRLF", "QMP socket")) {
return ERROR_FAIL;
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 1a4874c..19b4615 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -579,6 +579,110 @@ void libxl_cputopology_list_free(libxl_cputopology *list, int nr)
free(list);
}
+int libxl__sendmsg_fds(libxl__gc *gc, int carrier,
+ const void *data, size_t datalen,
+ int nfds, const int fds[], const char *what) {
+ struct msghdr msg = { 0 };
+ struct cmsghdr *cmsg;
+ size_t spaceneeded = nfds * sizeof(fds[0]);
+ char control[CMSG_SPACE(spaceneeded)];
+ struct iovec iov;
+ int r;
+
+ iov.iov_base = (void*)data;
+ iov.iov_len = datalen;
+
+ /* compose the message */
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ msg.msg_control = control;
+ msg.msg_controllen = sizeof(control);
+
+ /* attach open fd */
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ cmsg->cmsg_len = CMSG_LEN(spaceneeded);
+ memcpy(CMSG_DATA(cmsg), fds, spaceneeded);
+
+ msg.msg_controllen = cmsg->cmsg_len;
+
+ r = sendmsg(carrier, &msg, 0);
+ if (r < 0) {
+ LOGE(ERROR, "failed to send fd-carrying message (%s)", what);
+ return ERROR_FAIL;
+ }
+
+ return 0;
+}
+
+int libxl__recvmsg_fds(libxl__gc *gc, int carrier,
+ void *databuf, size_t datalen,
+ int nfds, int fds[], const char *what)
+{
+ struct msghdr msg = { 0 };
+ struct cmsghdr *cmsg;
+ size_t spaceneeded = nfds * sizeof(fds[0]);
+ char control[CMSG_SPACE(spaceneeded)];
+ struct iovec iov;
+ int r;
+
+ iov.iov_base = databuf;
+ iov.iov_len = datalen;
+
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ msg.msg_control = control;
+ msg.msg_controllen = sizeof(control);
+
+ for (;;) {
+ r = recvmsg(carrier, &msg, 0);
+ if (r < 0) {
+ if (errno == EINTR) continue;
+ if (errno == EWOULDBLOCK) return -1;
+ LOGE(ERROR,"recvmsg failed (%s)", what);
+ return ERROR_FAIL;
+ }
+ if (r == 0) {
+ LOG(ERROR,"recvmsg got EOF (%s)", what);
+ return ERROR_FAIL;
+ }
+ cmsg = CMSG_FIRSTHDR(&msg);
+ if (cmsg->cmsg_len <= CMSG_LEN(0)) {
+ LOG(ERROR,"recvmsg got no control msg"
+ " when expecting fds (%s)", what);
+ return ERROR_FAIL;
+ }
+ if (cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) {
+ LOG(ERROR, "recvmsg got unexpected"
+ " cmsg_level %d (!=%d) or _type %d (!=%d) (%s)",
+ cmsg->cmsg_level, SOL_SOCKET,
+ cmsg->cmsg_type, SCM_RIGHTS,
+ what);
+ return ERROR_FAIL;
+ }
+ if (cmsg->cmsg_len != CMSG_LEN(spaceneeded) ||
+ msg.msg_controllen != cmsg->cmsg_len) {
+ LOG(ERROR, "recvmsg got unexpected"
+ " number of fds or extra control data"
+ " (%ld bytes' worth, expected %ld) (%s)",
+ (long)CMSG_LEN(spaceneeded), (long)cmsg->cmsg_len,
+ what);
+ int i, fd;
+ unsigned char *p;
+ for (i=0, p=CMSG_DATA(cmsg);
+ CMSG_SPACE(i * sizeof(fds[0]));
+ i++, i+=sizeof(fd)) {
+ memcpy(&fd, p, sizeof(fd));
+ close(fd);
+ }
+ return ERROR_FAIL;
+ }
+ memcpy(fds, CMSG_DATA(cmsg), spaceneeded);
+ return 0;
+ }
+}
+
void libxl_dominfo_list_free(libxl_dominfo *list, int nr)
{
int i;
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 07/20] libxl: Clean up setdefault in do_domain_create
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (5 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 06/20] libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 08/20] libxl: provide libxl__datacopier_* Ian Jackson
` (12 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
do_domain_create called libxl__domain_create_info_setdefault twice.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
---
tools/libxl/libxl_create.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e63c7bd..3675afe 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -552,9 +552,6 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
if (ret) goto error_out;
- ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
- if (ret) goto error_out;
-
ret = libxl__domain_make(gc, &d_config->c_info, &domid);
if (ret) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot make domain: %d", ret);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 08/20] libxl: provide libxl__datacopier_*
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (6 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 07/20] libxl: Clean up setdefault in do_domain_create Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 09/20] libxl: provide libxl__openpty_* Ian Jackson
` (11 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
General facility for ao operations to shovel data between fds.
This will be used by the bootloader machinery.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/Makefile | 3 +-
tools/libxl/libxl_aoutils.c | 188 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 40 +++++++++
3 files changed, 230 insertions(+), 1 deletions(-)
create mode 100644 tools/libxl/libxl_aoutils.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 5ba144f..6e253b1 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -52,7 +52,8 @@ LIBXL_LIBS += -lyajl
LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
- libxl_internal.o libxl_utils.o libxl_uuid.o libxl_json.o \
+ libxl_internal.o libxl_utils.o libxl_uuid.o \
+ libxl_json.o libxl_aoutils.o \
libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
new file mode 100644
index 0000000..774fa03
--- /dev/null
+++ b/tools/libxl/libxl_aoutils.c
@@ -0,0 +1,188 @@
+/*
+ * Copyright (C) 2010 Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+
+#include "libxl_internal.h"
+
+/*----- data copier -----*/
+
+void libxl__datacopier_init(libxl__datacopier_state *dc)
+{
+ libxl__ev_fd_init(&dc->toread);
+ libxl__ev_fd_init(&dc->towrite);
+ LIBXL_TAILQ_INIT(&dc->bufs);
+}
+
+void libxl__datacopier_kill(libxl__datacopier_state *dc)
+{
+ STATE_AO_GC(dc->ao);
+ libxl__datacopier_buf *buf, *tbuf;
+
+ libxl__ev_fd_deregister(gc, &dc->toread);
+ libxl__ev_fd_deregister(gc, &dc->towrite);
+ LIBXL_TAILQ_FOREACH_SAFE(buf, &dc->bufs, entry, tbuf)
+ free(buf);
+ LIBXL_TAILQ_INIT(&dc->bufs);
+}
+
+static void datacopier_callback(libxl__egc *egc, libxl__datacopier_state *dc,
+ int onwrite, int errnoval)
+{
+ libxl__datacopier_kill(dc);
+ dc->callback(egc, dc, onwrite, errnoval);
+}
+
+static void datacopier_writable(libxl__egc *egc, libxl__ev_fd *ev,
+ int fd, short events, short revents);
+
+static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
+{
+ STATE_AO_GC(dc->ao);
+ int rc;
+
+ if (dc->used) {
+ if (!libxl__ev_fd_isregistered(&dc->towrite)) {
+ rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
+ dc->writefd, POLLOUT);
+ if (rc) {
+ LOG(ERROR, "unable to establish write event on %s"
+ " during copy of %s", dc->writewhat, dc->copywhat);
+ datacopier_callback(egc, dc, -1, 0);
+ return;
+ }
+ }
+ } else if (!libxl__ev_fd_isregistered(&dc->toread)) {
+ /* we have had eof */
+ datacopier_callback(egc, dc, 0, 0);
+ return;
+ } else {
+ /* nothing buffered, but still reading */
+ libxl__ev_fd_deregister(gc, &dc->towrite);
+ }
+}
+
+static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
+ int fd, short events, short revents) {
+ libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, toread);
+ STATE_AO_GC(dc->ao);
+
+ if (revents & ~POLLIN) {
+ LOG(ERROR, "unexpected poll event 0x%x (should be POLLIN)"
+ " on %s during copy of %s", revents, dc->readwhat, dc->copywhat);
+ datacopier_callback(egc, dc, -1, 0);
+ return;
+ }
+ assert(revents & POLLIN);
+ for (;;) {
+ while (dc->used >= dc->maxsz) {
+ libxl__datacopier_buf *rm = LIBXL_TAILQ_FIRST(&dc->bufs);
+ dc->used -= rm->used;
+ assert(dc->used >= 0);
+ LIBXL_TAILQ_REMOVE(&dc->bufs, rm, entry);
+ free(rm);
+ }
+
+ libxl__datacopier_buf *buf =
+ LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
+ if (!buf || buf->used >= sizeof(buf->buf)) {
+ buf = malloc(sizeof(*buf));
+ if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
+ buf->used = 0;
+ LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
+ }
+ int r = read(ev->fd,
+ buf->buf + buf->used,
+ sizeof(buf->buf) - buf->used);
+ if (r < 0) {
+ if (errno == EINTR) continue;
+ if (errno == EWOULDBLOCK) break;
+ LOGE(ERROR, "error reading %s during copy of %s",
+ dc->readwhat, dc->copywhat);
+ datacopier_callback(egc, dc, 0, errno);
+ return;
+ }
+ if (r == 0) {
+ libxl__ev_fd_deregister(gc, &dc->toread);
+ break;
+ }
+ buf->used += r;
+ dc->used += r;
+ assert(buf->used <= sizeof(buf->buf));
+ }
+ datacopier_check_state(egc, dc);
+}
+
+static void datacopier_writable(libxl__egc *egc, libxl__ev_fd *ev,
+ int fd, short events, short revents) {
+ libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, towrite);
+ STATE_AO_GC(dc->ao);
+
+ if (revents & ~POLLOUT) {
+ LOG(ERROR, "unexpected poll event 0x%x (should be POLLOUT)"
+ " on %s during copy of %s", revents, dc->writewhat, dc->copywhat);
+ datacopier_callback(egc, dc, -1, 0);
+ return;
+ }
+ assert(revents & POLLOUT);
+ for (;;) {
+ libxl__datacopier_buf *buf = LIBXL_TAILQ_FIRST(&dc->bufs);
+ if (!buf)
+ break;
+ if (!buf->used) {
+ LIBXL_TAILQ_REMOVE(&dc->bufs, buf, entry);
+ free(buf);
+ continue;
+ }
+ int r = write(ev->fd, buf->buf, buf->used);
+ if (r < 0) {
+ if (errno == EINTR) continue;
+ if (errno == EWOULDBLOCK) break;
+ LOGE(ERROR, "error writing to %s during copy of %s",
+ dc->writewhat, dc->copywhat);
+ datacopier_callback(egc, dc, 1, errno);
+ return;
+ }
+ assert(r > 0);
+ assert(r <= buf->used);
+ buf->used -= r;
+ dc->used -= r;
+ assert(dc->used >= 0);
+ memmove(buf->buf, buf->buf+r, buf->used);
+ }
+ datacopier_check_state(egc, dc);
+}
+
+int libxl__datacopier_start(libxl__datacopier_state *dc)
+{
+ int rc;
+ STATE_AO_GC(dc->ao);
+
+ libxl__datacopier_init(dc);
+
+ rc = libxl__ev_fd_register(gc, &dc->toread, datacopier_readable,
+ dc->readfd, POLLIN);
+ if (rc) goto out;
+
+ rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
+ dc->writefd, POLLOUT);
+ if (rc) goto out;
+
+ return 0;
+
+ out:
+ libxl__datacopier_kill(dc);
+ return rc;
+}
+
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6ceb362..20c95db 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -833,6 +833,7 @@ _hidden int libxl__ev_devstate_wait(libxl__gc *gc, libxl__ev_devstate *ds,
*/
_hidden int libxl__try_phy_backend(mode_t st_mode);
+
/* from libxl_pci */
_hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);
@@ -1458,6 +1459,45 @@ int libxl__carefd_close(libxl__carefd*);
int libxl__carefd_fd(const libxl__carefd*);
+/*----- datacopier: copies data from one fd to another -----*/
+
+typedef struct libxl__datacopier_state libxl__datacopier_state;
+typedef struct libxl__datacopier_buf libxl__datacopier_buf;
+
+/* onwrite==1 means failure happened when writing, logged, errnoval is valid
+ * onwrite==0 means failure happened when reading
+ * errnoval==0 means we got eof and all data was written
+ * errnoval!=0 means we had a read error, logged
+ * onwrite==-1 means some other internal failure, errnoval not valid, logged
+ * in all cases copier is killed before calling this callback */
+typedef void libxl__datacopier_callback(libxl__egc *egc,
+ libxl__datacopier_state *dc, int onwrite, int errnoval);
+
+struct libxl__datacopier_buf {
+ /* private to datacopier */
+ LIBXL_TAILQ_ENTRY(libxl__datacopier_buf) entry;
+ int used;
+ char buf[1000];
+};
+
+struct libxl__datacopier_state {
+ /* caller must fill these in, and they must all remain valid */
+ libxl__ao *ao;
+ int readfd, writefd;
+ ssize_t maxsz;
+ const char *copywhat, *readwhat, *writewhat; /* for error msgs */
+ libxl__datacopier_callback *callback;
+ /* remaining fields are private to datacopier */
+ libxl__ev_fd toread, towrite;
+ ssize_t used;
+ LIBXL_TAILQ_HEAD(libxl__datacopier_bufs, libxl__datacopier_buf) bufs;
+};
+
+_hidden void libxl__datacopier_init(libxl__datacopier_state *dc);
+_hidden void libxl__datacopier_kill(libxl__datacopier_state *dc);
+_hidden int libxl__datacopier_start(libxl__datacopier_state *dc);
+
+
/*
* Convenience macros.
*/
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 09/20] libxl: provide libxl__openpty_*
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (7 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 08/20] libxl: provide libxl__datacopier_* Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 10/20] libxl: ao: Convert libxl_run_bootloader Ian Jackson
` (10 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
General facility for ao operations to open ptys.
This will be used by the bootloader machinery.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_aoutils.c | 127 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 36 ++++++++++++
2 files changed, 163 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 774fa03..023de6e 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -186,3 +186,130 @@ int libxl__datacopier_start(libxl__datacopier_state *dc)
return rc;
}
+/*----- openpty -----*/
+
+/* implementation */
+
+static void openpty_cleanup(libxl__openpty_state *op)
+{
+ int i;
+
+ for (i=0; i<op->count; i++) {
+ libxl__openpty_result *res = &op->results[i];
+ libxl__carefd_close(res->master); res->master = 0;
+ libxl__carefd_close(res->slave); res->slave = 0;
+ }
+}
+
+static void openpty_exited(libxl__egc *egc, libxl__ev_child *child,
+ pid_t pid, int status) {
+ libxl__openpty_state *op = CONTAINER_OF(child, *op, child);
+ STATE_AO_GC(op->ao);
+
+ if (status) {
+ /* Perhaps the child gave us the fds and then exited nonzero.
+ * Well that would be odd but we don't really care. */
+ libxl_report_child_exitstatus(CTX, op->rc ? LIBXL__LOG_ERROR
+ : LIBXL__LOG_WARNING,
+ "openpty child", pid, status);
+ }
+ if (op->rc)
+ openpty_cleanup(op);
+ op->callback(egc, op);
+}
+
+int libxl__openptys(libxl__openpty_state *op,
+ const struct termios *termp,
+ const struct winsize *winp) {
+ /*
+ * This is completely crazy. openpty calls grantpt which the spec
+ * says may fork, and may not be called with a SIGCHLD handler.
+ * Now our application may have a SIGCHLD handler so that's bad.
+ * We could perhaps block it but we'd need to block it on all
+ * threads. This is just Too Hard.
+ *
+ * So instead, we run openpty in a child process. That child
+ * process then of course has only our own thread and our own
+ * signal handlers. We pass the fds back.
+ *
+ * Since our only current caller actually wants two ptys, we
+ * support calling openpty multiple times for a single fork.
+ */
+ STATE_AO_GC(op->ao);
+ int count = op->count;
+ int r, i, rc, sockets[2], ptyfds[count][2];
+ libxl__carefd *for_child = 0;
+ pid_t pid = -1;
+
+ for (i=0; i<count; i++) {
+ ptyfds[i][0] = ptyfds[i][1] = -1;
+ libxl__openpty_result *res = &op->results[i];
+ assert(!res->master);
+ assert(!res->slave);
+ }
+ sockets[0] = sockets[1] = -1; /* 0 is for us, 1 for our child */
+
+ libxl__carefd_begin();
+ r = socketpair(AF_UNIX, SOCK_STREAM, 0, sockets);
+ if (r) { sockets[0] = sockets[1] = -1; }
+ for_child = libxl__carefd_opened(CTX, sockets[1]);
+ if (r) { LOGE(ERROR,"socketpair failed"); rc = ERROR_FAIL; goto out; }
+
+ pid = libxl__ev_child_fork(gc, &op->child, openpty_exited);
+ if (pid == -1) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (!pid) {
+ /* child */
+ close(sockets[0]);
+ signal(SIGCHLD, SIG_DFL);
+
+ for (i=0; i<count; i++) {
+ r = openpty(&ptyfds[i][0], &ptyfds[i][1], NULL, termp, winp);
+ if (r) { LOGE(ERROR,"openpty failed"); _exit(-1); }
+ }
+ rc = libxl__sendmsg_fds(gc, sockets[1], "",1,
+ 2*count, &ptyfds[0][0], "ptys");
+ if (rc) { LOGE(ERROR,"sendmsg to parent failed"); _exit(-1); }
+ _exit(0);
+ }
+
+ libxl__carefd_close(for_child);
+ for_child = 0;
+
+ /* this should be fast so do it synchronously */
+
+ libxl__carefd_begin();
+ char buf[1];
+ rc = libxl__recvmsg_fds(gc, sockets[0], buf,1,
+ 2*count, &ptyfds[0][0], "ptys");
+ if (!rc) {
+ for (i=0; i<count; i++) {
+ libxl__openpty_result *res = &op->results[i];
+ res->master = libxl__carefd_record(CTX, ptyfds[i][0]);
+ res->slave = libxl__carefd_record(CTX, ptyfds[i][1]);
+ }
+ }
+ /* now the pty fds are in the carefds, if they were ever open */
+ libxl__carefd_unlock();
+ if (rc)
+ goto out;
+
+ rc = 0;
+
+ out:
+ if (sockets[0] >= 0) close(sockets[0]);
+ libxl__carefd_close(for_child);
+ if (libxl__ev_child_inuse(&op->child)) {
+ op->rc = rc;
+ /* we will get a callback when the child dies */
+ return 0;
+ }
+
+ assert(rc);
+ openpty_cleanup(op);
+ return rc;
+}
+
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 20c95db..9af99ec 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1498,6 +1498,42 @@ _hidden void libxl__datacopier_kill(libxl__datacopier_state *dc);
_hidden int libxl__datacopier_start(libxl__datacopier_state *dc);
+/*----- openpty -----*/
+
+/*
+ * opens count (>0) ptys like count calls to openpty, and then
+ * calls back. On entry, all op[].master and op[].slave must be
+ * 0. On callback, either rc==0 and master and slave are non-0,
+ * or rc is a libxl error and they are both 0. If libxl__openpty
+ * returns non-0 no callback will happen and everything is left
+ * cleaned up.
+ */
+
+typedef struct libxl__openpty_state libxl__openpty_state;
+typedef struct libxl__openpty_result libxl__openpty_result;
+typedef void libxl__openpty_callback(libxl__egc *egc, libxl__openpty_state *op);
+
+struct libxl__openpty_state {
+ /* caller must fill these in, and they must all remain valid */
+ libxl__ao *ao;
+ libxl__openpty_callback *callback;
+ int count;
+ libxl__openpty_result *results; /* actual size is count, out parameter */
+ /* public, result, caller may only read in callback */
+ int rc;
+ /* private for implementation */
+ libxl__ev_child child;
+};
+
+struct libxl__openpty_result {
+ libxl__carefd *master, *slave;
+};
+
+int libxl__openptys(libxl__openpty_state *op,
+ const struct termios *termp,
+ const struct winsize *winp);
+
+
/*
* Convenience macros.
*/
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 10/20] libxl: ao: Convert libxl_run_bootloader
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (8 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 09/20] libxl: provide libxl__openpty_* Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 11/20] libxl: make libxl_create_logfile const-correct Ian Jackson
` (9 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Convert libxl_run_bootloader to an ao_how-taking function.
It's implemented in terms of libxl__bootloader_run, which can be used
internally. The resulting code is pretty much a rewrite.
Significant changes include:
- We direct the bootloader's results to a file, not a pipe. This
makes it simpler to deal with as we don't have to read it
concurrently along with everything else.
- We now issue a warning if we find unexpected statements in the
bootloader results.
- The arrangements for buffering of bootloader input and output
are completely changed. Now we have a fixed limit of 64k
on output, and 4k on input, and discard the oldest data when
this overflows (which it shouldn't). There is no timeout
any more.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 4 +
tools/libxl/libxl.h | 3 +-
tools/libxl/libxl_bootloader.c | 708 +++++++++++++++++++++-------------------
tools/libxl/libxl_create.c | 8 +-
tools/libxl/libxl_internal.h | 34 ++
5 files changed, 415 insertions(+), 342 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 42ac89f..54f3813 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1748,6 +1748,10 @@ int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk)
* For other device types assume that the blktap2 process is
* needed by the soon to be started domain and do nothing.
*/
+ /*
+ * FIXME
+ * This appears to leak the disk in failure paths
+ */
return 0;
}
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 03e71f6..477b72a 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -494,7 +494,8 @@ int libxl_get_max_cpus(libxl_ctx *ctx);
int libxl_run_bootloader(libxl_ctx *ctx,
libxl_domain_build_info *info,
libxl_device_disk *disk,
- uint32_t domid);
+ uint32_t domid,
+ libxl_asyncop_how *ao_how);
/* 0 means ERROR_ENOMEM, which we have logged */
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index b50944a..6cccb6c 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -15,6 +15,7 @@
#include "libxl_osdeps.h" /* must come before any other headers */
#include <termios.h>
+#include <utmp.h>
#ifdef INCLUDE_LIBUTIL_H
#include INCLUDE_LIBUTIL_H
@@ -22,67 +23,80 @@
#include "libxl_internal.h"
-#define XENCONSOLED_BUF_SIZE 16
-#define BOOTLOADER_BUF_SIZE 4096
-#define BOOTLOADER_TIMEOUT 1
+#define BOOTLOADER_BUF_OUT 65536
+#define BOOTLOADER_BUF_IN 4096
-static char **make_bootloader_args(libxl__gc *gc,
- libxl_domain_build_info *info,
- uint32_t domid,
- const char *fifo, char *disk)
+static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op);
+static void bootloader_keystrokes_copyfail(libxl__egc *egc,
+ libxl__datacopier_state *dc, int onwrite, int errnoval);
+static void bootloader_display_copyfail(libxl__egc *egc,
+ libxl__datacopier_state *dc, int onwrite, int errnoval);
+static void bootloader_finished(libxl__egc *egc, libxl__ev_child *child,
+ pid_t pid, int status);
+
+/*----- bootloader arguments -----*/
+
+static void bootloader_arg(libxl__bootloader_state *bl, const char *arg)
+{
+ assert(bl->nargs < bl->argsspace);
+ bl->args[bl->nargs++] = arg;
+}
+
+static void make_bootloader_args(libxl__gc *gc, libxl__bootloader_state *bl)
{
- flexarray_t *args;
- int nr = 0;
+ const libxl_domain_build_info *info = bl->info;
- args = flexarray_make(1, 1);
- if (!args)
- return NULL;
+ bl->argsspace = 7 + libxl_string_list_length(&info->u.pv.bootloader_args);
- flexarray_set(args, nr++, (char *)info->u.pv.bootloader);
+ GCNEW_ARRAY(bl->args, bl->argsspace);
+
+#define ARG(arg) bootloader_arg(bl, (arg))
+
+ ARG(info->u.pv.bootloader);
if (info->u.pv.kernel.path)
- flexarray_set(args, nr++, libxl__sprintf(gc, "--kernel=%s",
- info->u.pv.kernel.path));
+ ARG(libxl__sprintf(gc, "--kernel=%s", info->u.pv.kernel.path));
if (info->u.pv.ramdisk.path)
- flexarray_set(args, nr++, libxl__sprintf(gc, "--ramdisk=%s", info->u.pv.ramdisk.path));
+ ARG(libxl__sprintf(gc, "--ramdisk=%s", info->u.pv.ramdisk.path));
if (info->u.pv.cmdline && *info->u.pv.cmdline != '\0')
- flexarray_set(args, nr++, libxl__sprintf(gc, "--args=%s", info->u.pv.cmdline));
+ ARG(libxl__sprintf(gc, "--args=%s", info->u.pv.cmdline));
- flexarray_set(args, nr++, libxl__sprintf(gc, "--output=%s", fifo));
- flexarray_set(args, nr++, "--output-format=simple0");
- flexarray_set(args, nr++, libxl__sprintf(gc, "--output-directory=%s", "/var/run/libxl/"));
+ ARG(libxl__sprintf(gc, "--output=%s", bl->outputpath));
+ ARG("--output-format=simple0");
+ ARG(libxl__sprintf(gc, "--output-directory=%s", bl->outputdir));
if (info->u.pv.bootloader_args) {
char **p = info->u.pv.bootloader_args;
while (*p) {
- flexarray_set(args, nr++, *p);
+ ARG(*p);
p++;
}
}
- flexarray_set(args, nr++, disk);
+ ARG(bl->diskpath);
- /* Sentinal for execv */
- flexarray_set(args, nr++, NULL);
+ /* Sentinel for execv */
+ ARG(NULL);
- return (char **) flexarray_contents(args); /* Frees args */
+#undef ARG
}
-static int open_xenconsoled_pty(int *master, int *slave, char *slave_path, size_t slave_path_len)
+/*----- synchronous subroutines -----*/
+
+static int setup_xenconsoled_pty(libxl__egc *egc, libxl__bootloader_state *bl,
+ char *slave_path, size_t slave_path_len)
{
+ STATE_AO_GC(bl->ao);
struct termios termattr;
- int ret;
-
- ret = openpty(master, slave, NULL, NULL, NULL);
- if (ret < 0)
- return -1;
-
- ret = ttyname_r(*slave, slave_path, slave_path_len);
- if (ret == -1) {
- close(*master);
- close(*slave);
- *master = *slave = -1;
- return -1;
+ int r, rc;
+ int slave = libxl__carefd_fd(bl->ptys[1].slave);
+ int master = libxl__carefd_fd(bl->ptys[1].master);
+
+ r = ttyname_r(slave, slave_path, slave_path_len);
+ if (r == -1) {
+ LOGE(ERROR,"ttyname_r failed");
+ rc = ERROR_FAIL;
+ goto out;
}
/*
@@ -95,310 +109,215 @@ static int open_xenconsoled_pty(int *master, int *slave, char *slave_path, size_
* semantics on Solaris, so don't try to set any attributes
* for it.
*/
-#if !defined(__sun__) && !defined(__NetBSD__)
- tcgetattr(*master, &termattr);
+ tcgetattr(master, &termattr);
cfmakeraw(&termattr);
- tcsetattr(*master, TCSANOW, &termattr);
-
- close(*slave);
- *slave = -1;
-#else
- tcgetattr(*slave, &termattr);
- cfmakeraw(&termattr);
- tcsetattr(*slave, TCSANOW, &termattr);
-#endif
-
- fcntl(*master, F_SETFL, O_NDELAY);
- fcntl(*master, F_SETFD, FD_CLOEXEC);
+ tcsetattr(master, TCSANOW, &termattr);
return 0;
+
+ out:
+ return rc;
}
-static pid_t fork_exec_bootloader(int *master, const char *arg0, char **args)
-{
- struct termios termattr;
- pid_t pid = forkpty(master, NULL, NULL, NULL);
- if (pid == -1)
- return -1;
- else if (pid == 0) {
- setenv("TERM", "vt100", 1);
- libxl__exec(-1, -1, -1, arg0, args);
- return -1;
- }
+static const char *bootloader_result_command(libxl__gc *gc, const char *buf,
+ const char *prefix, size_t prefixlen) {
+ if (strncmp(buf, prefix, prefixlen))
+ return 0;
- /*
- * On Solaris, the master pty side does not have terminal semantics,
- * so don't try to set any attributes, as it will fail.
- */
-#if !defined(__sun__)
- tcgetattr(*master, &termattr);
- cfmakeraw(&termattr);
- tcsetattr(*master, TCSANOW, &termattr);
-#endif
+ const char *rhs = buf + prefixlen;
+ if (!CTYPE(isspace,*rhs))
+ return 0;
+
+ while (CTYPE(isspace,*rhs))
+ rhs++;
- fcntl(*master, F_SETFL, O_NDELAY);
+ LOG(DEBUG,"bootloader output contained %s %s", prefix, rhs);
- return pid;
+ return rhs;
}
-/*
- * filedescriptors:
- * fifo_fd - bootstring output from the bootloader
- * xenconsoled_fd - input/output from/to xenconsole
- * bootloader_fd - input/output from/to pty that controls the bootloader
- * The filedescriptors are NDELAY, so it's ok to try to read
- * bigger chunks than may be available, to keep e.g. curses
- * screen redraws in the bootloader efficient. xenconsoled_fd is the side that
- * gets xenconsole input, which will be keystrokes, so a small number
- * is sufficient. bootloader_fd is pygrub output, which will be curses screen
- * updates, so a larger number (1024) is appropriate there.
- *
- * For writeable descriptors, only include them in the set for select
- * if there is actual data to write, otherwise this would loop too fast,
- * eating up CPU time.
- */
-static char * bootloader_interact(libxl__gc *gc, int xenconsoled_fd, int bootloader_fd, int fifo_fd)
+static int parse_bootloader_result(libxl__egc *egc,
+ libxl__bootloader_state *bl)
{
- int ret;
-
- size_t nr_out = 0, size_out = 0;
- char *output = NULL;
- struct timeval wait;
-
- /* input from xenconsole. read on xenconsoled_fd write to bootloader_fd */
- int xenconsoled_prod = 0, xenconsoled_cons = 0;
- char xenconsoled_buf[XENCONSOLED_BUF_SIZE];
- /* output from bootloader. read on bootloader_fd write to xenconsoled_fd */
- int bootloader_prod = 0, bootloader_cons = 0;
- char bootloader_buf[BOOTLOADER_BUF_SIZE];
-
- while(1) {
- fd_set wsel, rsel;
- int nfds;
-
- /* Set timeout to 1s before starting to discard data */
- wait.tv_sec = BOOTLOADER_TIMEOUT;
- wait.tv_usec = 0;
-
- /* Move buffers around to drop already consumed data */
- if (xenconsoled_cons > 0) {
- xenconsoled_prod -= xenconsoled_cons;
- memmove(xenconsoled_buf, &xenconsoled_buf[xenconsoled_cons],
- xenconsoled_prod);
- xenconsoled_cons = 0;
- }
- if (bootloader_cons > 0) {
- bootloader_prod -= bootloader_cons;
- memmove(bootloader_buf, &bootloader_buf[bootloader_cons],
- bootloader_prod);
- bootloader_cons = 0;
- }
-
- FD_ZERO(&rsel);
- FD_SET(fifo_fd, &rsel);
- nfds = fifo_fd + 1;
- if (xenconsoled_prod < XENCONSOLED_BUF_SIZE) {
- /* The buffer is not full, try to read more data */
- FD_SET(xenconsoled_fd, &rsel);
- nfds = xenconsoled_fd + 1 > nfds ? xenconsoled_fd + 1 : nfds;
- }
- if (bootloader_prod < BOOTLOADER_BUF_SIZE) {
- /* The buffer is not full, try to read more data */
- FD_SET(bootloader_fd, &rsel);
- nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
- }
-
- FD_ZERO(&wsel);
- if (bootloader_prod > 0) {
- /* The buffer has data to consume */
- FD_SET(xenconsoled_fd, &wsel);
- nfds = xenconsoled_fd + 1 > nfds ? xenconsoled_fd + 1 : nfds;
- }
- if (xenconsoled_prod > 0) {
- /* The buffer has data to consume */
- FD_SET(bootloader_fd, &wsel);
- nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
- }
-
- if (xenconsoled_prod == XENCONSOLED_BUF_SIZE ||
- bootloader_prod == BOOTLOADER_BUF_SIZE)
- ret = select(nfds, &rsel, &wsel, NULL, &wait);
- else
- ret = select(nfds, &rsel, &wsel, NULL, NULL);
- if (ret < 0) {
- if (errno == EINTR)
- continue;
- goto out_err;
- }
-
- /* Input from xenconsole, read xenconsoled_fd, write bootloader_fd */
- if (ret == 0 && xenconsoled_prod == XENCONSOLED_BUF_SIZE) {
- /* Drop the buffer */
- xenconsoled_prod = 0;
- xenconsoled_cons = 0;
- } else if (FD_ISSET(xenconsoled_fd, &rsel)) {
- ret = read(xenconsoled_fd, &xenconsoled_buf[xenconsoled_prod], XENCONSOLED_BUF_SIZE - xenconsoled_prod);
- if (ret < 0 && errno != EIO && errno != EAGAIN)
- goto out_err;
- if (ret > 0)
- xenconsoled_prod += ret;
- }
- if (FD_ISSET(bootloader_fd, &wsel)) {
- ret = write(bootloader_fd, &xenconsoled_buf[xenconsoled_cons], xenconsoled_prod - xenconsoled_cons);
- if (ret < 0 && errno != EIO && errno != EAGAIN)
- goto out_err;
- if (ret > 0)
- xenconsoled_cons += ret;
- }
+ STATE_AO_GC(bl->ao);
+ char buf[PATH_MAX*2];
+ FILE *f = 0;
+ int rc = ERROR_FAIL;
+ libxl_domain_build_info *info = bl->info;
+
+ f = fopen(bl->outputpath, "r");
+ if (!f) {
+ LOGE(ERROR,"open bootloader output file %s", bl->outputpath);
+ goto out;
+ }
- /* Input from bootloader, read bootloader_fd, write xenconsoled_fd */
- if (ret == 0 && bootloader_prod == BOOTLOADER_BUF_SIZE) {
- /* Drop the buffer */
- bootloader_prod = 0;
- bootloader_cons = 0;
- } else if (FD_ISSET(bootloader_fd, &rsel)) {
- ret = read(bootloader_fd, &bootloader_buf[bootloader_prod], BOOTLOADER_BUF_SIZE - bootloader_prod);
- if (ret < 0 && errno != EIO && errno != EAGAIN)
- goto out_err;
- if (ret > 0)
- bootloader_prod += ret;
- }
- if (FD_ISSET(xenconsoled_fd, &wsel)) {
- ret = write(xenconsoled_fd, &bootloader_buf[bootloader_cons], bootloader_prod - bootloader_cons);
- if (ret < 0 && errno != EIO && errno != EAGAIN)
- goto out_err;
- if (ret > 0)
- bootloader_cons += ret;
+ for (;;) {
+ /* Read a nul-terminated "line" and put the result in
+ * buf, and its length (not including the nul) in l */
+ int l = 0, c;
+ while ((c = getc(f)) != EOF && c != '\0') {
+ if (l < sizeof(buf)-1)
+ buf[l] = c;
+ l++;
}
-
- if (FD_ISSET(fifo_fd, &rsel)) {
- if (size_out - nr_out < 256) {
- char *temp;
- size_t new_size = size_out == 0 ? 32 : size_out * 2;
-
- temp = realloc(output, new_size);
- if (temp == NULL)
- goto out_err;
- output = temp;
- memset(output + size_out, 0, new_size - size_out);
- size_out = new_size;
+ if (c == EOF) {
+ if (ferror(f)) {
+ LOGE(ERROR,"read bootloader output file %s", bl->outputpath);
+ goto out;
}
-
- ret = read(fifo_fd, output + nr_out, size_out - nr_out);
- if (ret > 0)
- nr_out += ret;
- if (ret == 0)
+ if (!l)
break;
}
- }
-
- libxl__ptr_add(gc, output);
- return output;
+ if (l >= sizeof(buf)) {
+ LOG(WARN,"bootloader output contained"
+ " overly long item `%.150s...'", buf);
+ continue;
+ }
+ buf[l] = 0;
-out_err:
- free(output);
- return NULL;
-}
+ const char *rhs;
+#define COMMAND(s) ((rhs = bootloader_result_command(gc, buf, s, sizeof(s)-1)))
-static void parse_bootloader_result(libxl__gc *gc,
- libxl_domain_build_info *info,
- const char *o)
-{
- while (*o != '\0') {
- if (strncmp("kernel ", o, strlen("kernel ")) == 0) {
+ if (COMMAND("kernel")) {
free(info->u.pv.kernel.path);
- info->u.pv.kernel.path = strdup(o + strlen("kernel "));
+ info->u.pv.kernel.path = libxl__strdup(NULL, rhs);
libxl__file_reference_map(&info->u.pv.kernel);
unlink(info->u.pv.kernel.path);
- } else if (strncmp("ramdisk ", o, strlen("ramdisk ")) == 0) {
+ } else if (COMMAND("ramdisk")) {
free(info->u.pv.ramdisk.path);
- info->u.pv.ramdisk.path = strdup(o + strlen("ramdisk "));
+ info->u.pv.ramdisk.path = libxl__strdup(NULL, rhs);
libxl__file_reference_map(&info->u.pv.ramdisk);
unlink(info->u.pv.ramdisk.path);
- } else if (strncmp("args ", o, strlen("args ")) == 0) {
+ } else if (COMMAND("args")) {
free(info->u.pv.cmdline);
- info->u.pv.cmdline = strdup(o + strlen("args "));
+ info->u.pv.cmdline = libxl__strdup(NULL, rhs);
+ } else if (l) {
+ LOG(WARN, "unexpected output from bootloader: `%s'", buf);
}
-
- o = o + strlen(o) + 1;
}
+ rc = 0;
+
+ out:
+ if (f) fclose(f);
+ return rc;
}
-int libxl_run_bootloader(libxl_ctx *ctx,
- libxl_domain_build_info *info,
- libxl_device_disk *disk,
- uint32_t domid)
+
+/*----- init and cleanup -----*/
+
+void libxl__bootloader_init(libxl__bootloader_state *bl)
+{
+ bl->diskpath = NULL;
+ bl->ptys[0].master = bl->ptys[0].slave = 0;
+ bl->ptys[1].master = bl->ptys[1].slave = 0;
+ libxl__ev_child_init(&bl->child);
+ libxl__datacopier_init(&bl->keystrokes);
+ libxl__datacopier_init(&bl->display);
+}
+
+static void bootloader_cleanup(libxl__egc *egc, libxl__bootloader_state *bl)
{
- GC_INIT(ctx);
- int ret, rc = 0;
- char *fifo = NULL;
- char *diskpath = NULL;
- char **args = NULL;
+ STATE_AO_GC(bl->ao);
+ int i;
- char tempdir_template[] = "/var/run/libxl/bl.XXXXXX";
- char *tempdir;
+ if (bl->outputpath) libxl__remove_file(gc, bl->outputpath);
+ if (bl->outputdir) libxl__remove_directory(gc, bl->outputdir);
- char *dom_console_xs_path;
- char dom_console_slave_tty_path[PATH_MAX];
+ if (bl->diskpath) {
+ libxl_device_disk_local_detach(CTX, bl->disk);
+ free(bl->diskpath);
+ bl->diskpath = 0;
+ }
+ libxl__datacopier_kill(&bl->keystrokes);
+ libxl__datacopier_kill(&bl->display);
+ for (i=0; i<2; i++) {
+ libxl__carefd_close(bl->ptys[i].master);
+ libxl__carefd_close(bl->ptys[i].slave);
+ }
+}
+
+static void bootloader_setpaths(libxl__gc *gc, libxl__bootloader_state *bl)
+{
+ uint32_t domid = bl->domid;
+ bl->outputdir = GCSPRINTF(XEN_RUN_DIR "/bootloader.%"PRIu32".d", domid);
+ bl->outputpath = GCSPRINTF(XEN_RUN_DIR "/bootloader.%"PRIu32".out", domid);
+}
- int xenconsoled_fd = -1, xenconsoled_slave = -1;
- int bootloader_fd = -1, fifo_fd = -1;
+static void bootloader_callback(libxl__egc *egc, libxl__bootloader_state *bl,
+ int rc)
+{
+ bootloader_cleanup(egc, bl);
+ bl->callback(egc, bl, rc);
+}
- int blrc;
- pid_t pid;
- char *blout;
+/*----- main flow of control -----*/
- struct stat st_buf;
+void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
+{
+ STATE_AO_GC(bl->ao);
+ libxl_domain_build_info *info = bl->info;
+ int rc, r;
- rc = libxl__domain_build_info_setdefault(gc, info);
- if (rc) goto out;
+ libxl__bootloader_init(bl);
- if (info->type != LIBXL_DOMAIN_TYPE_PV || !info->u.pv.bootloader)
+ if (info->type != LIBXL_DOMAIN_TYPE_PV || !info->u.pv.bootloader) {
+ bl->callback(egc, bl, 0);
+ rc = 0;
goto out;
+ }
- rc = libxl__domain_build_info_setdefault(gc, info);
- if (rc) goto out;
+ bootloader_setpaths(gc, bl);
- rc = ERROR_INVAL;
- if (!disk)
+ for (;;) {
+ r = mkdir(bl->outputdir, 0600);
+ if (!r) break;
+ if (errno == EINTR) continue;
+ if (errno == EEXIST) break;
+ LOGE(ERROR, "failed to create bootloader dir %s", bl->outputdir);
+ rc = ERROR_FAIL;
goto out;
+ }
- rc = ERROR_FAIL;
- ret = mkdir("/var/run/libxl/", S_IRWXU);
- if (ret < 0 && errno != EEXIST)
+ for (;;) {
+ r = open(bl->outputpath, O_WRONLY|O_CREAT|O_TRUNC, 0600);
+ if (r>=0) { close(r); break; }
+ if (errno == EINTR) continue;
+ LOGE(ERROR, "failed to precreate bootloader output %s", bl->outputpath);
+ rc = ERROR_FAIL;
goto out;
+ }
- ret = stat("/var/run/libxl/", &st_buf);
- if (ret < 0)
+ bl->diskpath = libxl_device_disk_local_attach(CTX, bl->disk);
+ if (!bl->diskpath) {
+ rc = ERROR_FAIL;
goto out;
+ }
- if (!S_ISDIR(st_buf.st_mode))
- goto out;
+ make_bootloader_args(gc, bl);
- tempdir = mkdtemp(tempdir_template);
- if (tempdir == NULL)
- goto out;
+ bl->openpty.ao = ao;
+ bl->openpty.callback = bootloader_gotptys;
+ bl->openpty.count = 2;
+ bl->openpty.results = bl->ptys;
+ rc = libxl__openptys(&bl->openpty, 0,0);
+ if (rc) goto out;
- ret = asprintf(&fifo, "%s/fifo", tempdir);
- if (ret < 0) {
- fifo = NULL;
- goto out_close;
- }
+ return;
- ret = mkfifo(fifo, 0600);
- if (ret < 0) {
- goto out_close;
- }
+ out:
+ assert(rc);
+ bootloader_callback(egc, bl, rc);
+}
- diskpath = libxl_device_disk_local_attach(ctx, disk);
- if (!diskpath) {
- goto out_close;
- }
+static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
+{
+ libxl__bootloader_state *bl = CONTAINER_OF(op, *bl, openpty);
+ STATE_AO_GC(bl->ao);
+ int rc, r;
- args = make_bootloader_args(gc, info, domid, fifo, diskpath);
- if (args == NULL) {
- rc = ERROR_NOMEM;
- goto out_close;
+ if (bl->openpty.rc) {
+ rc = bl->openpty.rc;
+ goto out;
}
/*
@@ -411,76 +330,187 @@ int libxl_run_bootloader(libxl_ctx *ctx,
* where we copy characters between the two master fds, as well as
* listening on the bootloader's fifo for the results.
*/
- ret = open_xenconsoled_pty(&xenconsoled_fd, &xenconsoled_slave,
+
+ char *dom_console_xs_path;
+ char dom_console_slave_tty_path[PATH_MAX];
+ rc = setup_xenconsoled_pty(egc, bl,
&dom_console_slave_tty_path[0],
sizeof(dom_console_slave_tty_path));
- if (ret < 0) {
- goto out_close;
+ if (rc) goto out;
+
+ char *dompath = libxl__xs_get_dompath(gc, bl->domid);
+ if (!dompath) { rc = ERROR_FAIL; goto out; }
+
+ dom_console_xs_path = GCSPRINTF("%s/console/tty", dompath);
+
+ rc = libxl__xs_write(gc, XBT_NULL, dom_console_xs_path, "%s",
+ dom_console_slave_tty_path);
+ if (rc) {
+ LOGE(ERROR,"xs write console path %s := %s failed",
+ dom_console_xs_path, dom_console_slave_tty_path);
+ rc = ERROR_FAIL;
+ goto out;
}
- dom_console_xs_path = libxl__sprintf(gc, "%s/console/tty", libxl__xs_get_dompath(gc, domid));
- libxl__xs_write(gc, XBT_NULL, dom_console_xs_path, "%s", dom_console_slave_tty_path);
+ int bootloader_master = libxl__carefd_fd(bl->ptys[0].master);
+ int xenconsole_master = libxl__carefd_fd(bl->ptys[1].master);
+
+ libxl_fd_set_nonblock(CTX, bootloader_master, 1);
+ libxl_fd_set_nonblock(CTX, xenconsole_master, 1);
+
+ bl->keystrokes.writefd = bl->display.readfd = bootloader_master;
+ bl->keystrokes.writewhat = bl->display.readwhat = "bootloader pty";
+
+ bl->keystrokes.readfd = bl->display.writefd = xenconsole_master;
+ bl->keystrokes.readwhat = bl->display.writewhat = "xenconsole client pty";
+
+ bl->keystrokes.ao = ao;
+ bl->keystrokes.maxsz = BOOTLOADER_BUF_OUT;
+ bl->keystrokes.copywhat =
+ GCSPRINTF("bootloader input for domain %"PRIu32, bl->domid);
+ bl->keystrokes.callback = bootloader_keystrokes_copyfail;
+ rc = libxl__datacopier_start(&bl->keystrokes);
+ if (rc) goto out;
+
+ bl->display.ao = ao;
+ bl->display.maxsz = BOOTLOADER_BUF_IN;
+ bl->display.copywhat =
+ GCSPRINTF("bootloader output for domain %"PRIu32, bl->domid);
+ bl->display.callback = bootloader_display_copyfail;
+ rc = libxl__datacopier_start(&bl->display);
+ if (rc) goto out;
+
+ LOG(DEBUG, "executing bootloader: %s", bl->args[0]);
+ for (const char **blarg = bl->args;
+ *blarg;
+ blarg++)
+ LOG(DEBUG, " bootloader arg: %s", *blarg);
+
+ struct termios termattr;
- pid = fork_exec_bootloader(&bootloader_fd, info->u.pv.bootloader, args);
- if (pid < 0) {
- goto out_close;
+ pid_t pid = libxl__ev_child_fork(gc, &bl->child, bootloader_finished);
+ if (pid == -1) {
+ rc = ERROR_FAIL;
+ goto out;
}
- while (1) {
- if (waitpid(pid, &blrc, WNOHANG) == pid)
- goto out_close;
+ if (!pid) {
+ /* child */
+ r = login_tty(libxl__carefd_fd(bl->ptys[0].slave));
+ if (r) { LOGE(ERROR, "login_tty failed"); exit(-1); }
+ setenv("TERM", "vt100", 1);
+ libxl__exec(-1, -1, -1, bl->args[0], (char**)bl->args);
+ exit(-1);
+ }
- fifo_fd = open(fifo, O_RDONLY);
- if (fifo_fd > -1)
- break;
+ /* parent */
- if (errno == EINTR)
- continue;
+ /*
+ * On Solaris, the master pty side does not have terminal semantics,
+ * so don't try to set any attributes, as it will fail.
+ */
+#if !defined(__sun__)
+ tcgetattr(bootloader_master, &termattr);
+ cfmakeraw(&termattr);
+ tcsetattr(bootloader_master, TCSANOW, &termattr);
+#endif
- goto out_close;
+ return;
+
+ out:
+ bootloader_callback(egc, bl, rc);
+}
+
+/* perhaps one of these will be called, but perhaps not */
+static void bootloader_copyfail(libxl__egc *egc, const char *which,
+ libxl__bootloader_state *bl, int onwrite, int errnoval)
+{
+ STATE_AO_GC(bl->ao);
+ int r;
+
+ if (!onwrite && !errnoval)
+ LOG(ERROR, "unexpected eof copying %s", which);
+ libxl__datacopier_kill(&bl->keystrokes);
+ libxl__datacopier_kill(&bl->display);
+ if (bl->child.pid >= 0) {
+ r = kill(bl->child.pid, SIGTERM);
+ if (r) LOGE(WARN, "after failure, failed to kill bootloader [%lu]",
+ (unsigned long)bl->child.pid);
}
+ bl->rc = ERROR_FAIL;
+}
+static void bootloader_keystrokes_copyfail(libxl__egc *egc,
+ libxl__datacopier_state *dc, int onwrite, int errnoval)
+{
+ libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, keystrokes);
+ bootloader_copyfail(egc, "bootloader input", bl, onwrite, errnoval);
+}
+static void bootloader_display_copyfail(libxl__egc *egc,
+ libxl__datacopier_state *dc, int onwrite, int errnoval)
+{
+ libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, display);
+ bootloader_copyfail(egc, "bootloader output", bl, onwrite, errnoval);
+}
- fcntl(fifo_fd, F_SETFL, O_NDELAY);
+static void bootloader_finished(libxl__egc *egc, libxl__ev_child *child,
+ pid_t pid, int status) {
+ libxl__bootloader_state *bl = CONTAINER_OF(child, *bl, child);
+ STATE_AO_GC(bl->ao);
+ int rc;
- blout = bootloader_interact(gc, xenconsoled_fd, bootloader_fd, fifo_fd);
- if (blout == NULL) {
- goto out_close;
+ libxl__datacopier_kill(&bl->keystrokes);
+ libxl__datacopier_kill(&bl->display);
+
+ if (status) {
+ libxl_report_child_exitstatus(CTX, XTL_ERROR, "bootloader",
+ pid, status);
+ rc = ERROR_FAIL;
+ goto out;
+ } else {
+ LOG(DEBUG, "bootloader completed");
}
- pid = waitpid(pid, &blrc, 0);
- if (pid == -1 || (pid > 0 && WIFEXITED(blrc) && WEXITSTATUS(blrc) != 0)) {
- goto out_close;
+ if (bl->rc) {
+ /* datacopier went wrong */
+ rc = bl->rc;
+ goto out;
}
- parse_bootloader_result(gc, info, blout);
+ rc = parse_bootloader_result(egc, bl);
+ if (rc) goto out;
rc = 0;
-out_close:
- if (diskpath) {
- libxl_device_disk_local_detach(ctx, disk);
- free(diskpath);
- }
- if (fifo_fd > -1)
- close(fifo_fd);
- if (bootloader_fd > -1)
- close(bootloader_fd);
- if (xenconsoled_fd > -1)
- close(xenconsoled_fd);
- if (xenconsoled_slave > -1)
- close(xenconsoled_slave);
-
- if (fifo) {
- unlink(fifo);
- free(fifo);
- }
+ LOG(DEBUG, "bootloader execution successful");
- rmdir(tempdir);
+ out:
+ bootloader_callback(egc, bl, rc);
+}
- free(args);
+/*----- entrypoint for external callers -----*/
-out:
- GC_FREE;
- return rc;
+static void run_bootloader_done(libxl__egc *egc,
+ libxl__bootloader_state *st, int rc)
+{
+ libxl__ao_complete(egc, st->ao, rc);
+}
+
+int libxl_run_bootloader(libxl_ctx *ctx,
+ libxl_domain_build_info *info,
+ libxl_device_disk *disk,
+ uint32_t domid,
+ libxl_asyncop_how *ao_how)
+{
+ AO_CREATE(ctx,domid,ao_how);
+ libxl__bootloader_state *bl;
+
+ GCNEW(bl);
+ bl->ao = ao;
+ bl->callback = run_bootloader_done;
+ bl->info = info;
+ bl->disk = disk;
+ bl->domid = domid;
+ libxl__bootloader_run(egc, bl);
+ return AO_INPROGRESS;
}
/*
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 3675afe..dbc3cf0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -572,8 +572,12 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
if (ret) goto error_out;
}
- if ( restore_fd < 0 ) {
- ret = libxl_run_bootloader(ctx, &d_config->b_info, d_config->num_disks > 0 ? &d_config->disks[0] : NULL, domid);
+ libxl_device_disk *bootdisk =
+ d_config->num_disks > 0 ? &d_config->disks[0] : NULL;
+
+ if (restore_fd < 0 && bootdisk) {
+ ret = libxl_run_bootloader(ctx, &d_config->b_info, bootdisk, domid,
+ 0 /* fixme-ao */);
if (ret) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
"failed to run bootloader: %d", ret);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9af99ec..b3f84ba 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -42,6 +42,7 @@
#include <sys/time.h>
#include <sys/types.h>
#include <sys/wait.h>
+#include <sys/socket.h>
#include <xs.h>
#include <xenctrl.h>
@@ -449,6 +450,7 @@ _hidden int libxl__remove_file(libxl__gc *gc, const char *path);
_hidden int libxl__remove_directory(libxl__gc *gc, const char *path);
_hidden int libxl__remove_file_or_directory(libxl__gc *gc, const char *path);
+
_hidden char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array, int length);
_hidden int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
@@ -1534,6 +1536,38 @@ int libxl__openptys(libxl__openpty_state *op,
const struct winsize *winp);
+/*----- bootloader -----*/
+
+typedef struct libxl__bootloader_state libxl__bootloader_state;
+typedef void libxl__run_bootloader_callback(libxl__egc*,
+ libxl__bootloader_state*, int rc);
+
+struct libxl__bootloader_state {
+ /* caller must fill these in, and they must all remain valid */
+ libxl__ao *ao;
+ libxl__run_bootloader_callback *callback;
+ libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated */
+ libxl_device_disk *disk;
+ uint32_t domid;
+ /* private to libxl__run_bootloader */
+ char *outputpath, *outputdir;
+ char *diskpath; /* not from gc, represents actually attached disk */
+ libxl__openpty_state openpty;
+ libxl__openpty_result ptys[2]; /* [0] is for bootloader */
+ libxl__ev_child child;
+ int nargs, argsspace;
+ const char **args;
+ libxl__datacopier_state keystrokes, display;
+ int rc;
+};
+
+_hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
+
+/* Will definitely call st->callback, perhaps reentrantly.
+ * If callback is passed rc==0, will have updated st->info appropriately */
+_hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st);
+
+
/*
* Convenience macros.
*/
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 11/20] libxl: make libxl_create_logfile const-correct
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (9 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 10/20] libxl: ao: Convert libxl_run_bootloader Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 12/20] libxl: log bootloader output Ian Jackson
` (8 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_utils.c | 2 +-
tools/libxl/libxl_utils.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 19b4615..f0d94c6 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -193,7 +193,7 @@ static int logrename(libxl__gc *gc, const char *old, const char *new)
return 0;
}
-int libxl_create_logfile(libxl_ctx *ctx, char *name, char **full_name)
+int libxl_create_logfile(libxl_ctx *ctx, const char *name, char **full_name)
{
GC_INIT(ctx);
struct stat stat_buf;
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index ca53a8a..2b47622 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -26,7 +26,7 @@ int libxl_name_to_cpupoolid(libxl_ctx *ctx, const char *name, uint32_t *poolid);
char *libxl_cpupoolid_to_name(libxl_ctx *ctx, uint32_t poolid);
int libxl_get_stubdom_id(libxl_ctx *ctx, int guest_domid);
int libxl_is_stubdom(libxl_ctx *ctx, uint32_t domid, uint32_t *target_domid);
-int libxl_create_logfile(libxl_ctx *ctx, char *name, char **full_name);
+int libxl_create_logfile(libxl_ctx *ctx, const char *name, char **full_name);
int libxl_string_to_backend(libxl_ctx *ctx, char *s, libxl_disk_backend *backend);
int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 12/20] libxl: log bootloader output
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (10 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 11/20] libxl: make libxl_create_logfile const-correct Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 13/20] libxl: Allow AO_GC and EGC_GC even if not used Ian Jackson
` (7 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
This involves adding a new log feature to libxl__datacopier, and then
using it.
If the bootloader exits nonzero we print the log filename in a log
message from libxl.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_aoutils.c | 10 ++++++++++
tools/libxl/libxl_bootloader.c | 24 ++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 3 ++-
3 files changed, 36 insertions(+), 1 deletions(-)
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 023de6e..9b30544 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -117,6 +117,16 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
libxl__ev_fd_deregister(gc, &dc->toread);
break;
}
+ if (dc->log) {
+ int wrote = fwrite(buf->buf + buf->used, 1, r, dc->log);
+ if (wrote != r) {
+ assert(ferror(dc->log));
+ assert(errno);
+ LOGE(ERROR, "error logging %s", dc->copywhat);
+ datacopier_callback(egc, dc, 0, errno);
+ return;
+ }
+ }
buf->used += r;
dc->used += r;
assert(buf->used <= sizeof(buf->buf));
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 6cccb6c..eaec46a 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -234,6 +234,10 @@ static void bootloader_cleanup(libxl__egc *egc, libxl__bootloader_state *bl)
libxl__carefd_close(bl->ptys[i].master);
libxl__carefd_close(bl->ptys[i].slave);
}
+ if (bl->display.log) {
+ fclose(bl->display.log);
+ bl->display.log = NULL;
+ }
}
static void bootloader_setpaths(libxl__gc *gc, libxl__bootloader_state *bl)
@@ -256,6 +260,8 @@ void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
{
STATE_AO_GC(bl->ao);
libxl_domain_build_info *info = bl->info;
+ uint32_t domid = bl->domid;
+ char *logfile_tmp = NULL;
int rc, r;
libxl__bootloader_init(bl);
@@ -268,6 +274,22 @@ void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
bootloader_setpaths(gc, bl);
+ const char *logfile_leaf = GCSPRINTF("bootloader.%"PRIu32, domid);
+ rc = libxl_create_logfile(CTX, logfile_leaf, &logfile_tmp);
+ if (rc) goto out;
+
+ /* Transfer ownership of log filename to bl and the gc */
+ bl->logfile = logfile_tmp;
+ libxl__ptr_add(gc, logfile_tmp);
+ logfile_tmp = NULL;
+
+ bl->display.log = fopen(bl->logfile, "a");
+ if (!bl->display.log) {
+ LOGE(ERROR, "failed to create bootloader logfile %s", bl->logfile);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
for (;;) {
r = mkdir(bl->outputdir, 0600);
if (!r) break;
@@ -305,6 +327,7 @@ void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
return;
out:
+ free(logfile_tmp);
assert(rc);
bootloader_callback(egc, bl, rc);
}
@@ -462,6 +485,7 @@ static void bootloader_finished(libxl__egc *egc, libxl__ev_child *child,
libxl__datacopier_kill(&bl->display);
if (status) {
+ LOG(ERROR, "bootloader failed - consult logfile %s", bl->logfile);
libxl_report_child_exitstatus(CTX, XTL_ERROR, "bootloader",
pid, status);
rc = ERROR_FAIL;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b3f84ba..4cfb8d5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1488,6 +1488,7 @@ struct libxl__datacopier_state {
int readfd, writefd;
ssize_t maxsz;
const char *copywhat, *readwhat, *writewhat; /* for error msgs */
+ FILE *log; /* gets a copy of everything */
libxl__datacopier_callback *callback;
/* remaining fields are private to datacopier */
libxl__ev_fd toread, towrite;
@@ -1550,7 +1551,7 @@ struct libxl__bootloader_state {
libxl_device_disk *disk;
uint32_t domid;
/* private to libxl__run_bootloader */
- char *outputpath, *outputdir;
+ char *outputpath, *outputdir, *logfile;
char *diskpath; /* not from gc, represents actually attached disk */
libxl__openpty_state openpty;
libxl__openpty_result ptys[2]; /* [0] is for bootloader */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 13/20] libxl: Allow AO_GC and EGC_GC even if not used
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (11 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 12/20] libxl: log bootloader output Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-16 14:43 ` Ian Campbell
2012-04-13 18:40 ` [PATCH 14/20] libxl: remove ctx->waitpid_instead Ian Jackson
` (6 subsequent siblings)
19 siblings, 1 reply; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Mark the gc produced by AO_GC and EGC_GC with the gcc feature
__attribute__((unused)). This allows the use of EGC_INIT and
STATE_AO_GC by functions which do actually use the gc.
This is convenient because those functions might want to use the ao or
egc, rather than the gc; and also because it means that functions
which morally ought to be fishing any gc they use out of an egc or
state structure can be written do so regardless of whether the gc is
actually used right then.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_internal.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4cfb8d5..74dc2c5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1280,7 +1280,7 @@ libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
/* useful for all functions which take an egc: */
#define EGC_GC \
- libxl__gc *const gc = &egc->gc
+ libxl__gc *const gc __attribute__((unused)) = &egc->gc
/* egc initialisation and destruction: */
@@ -1383,7 +1383,7 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
})
#define AO_GC \
- libxl__gc *const gc = &ao->gc
+ libxl__gc *const gc __attribute__((unused)) = &ao->gc
#define STATE_AO_GC(op_ao) \
libxl__ao *const ao = (op_ao); \
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 14/20] libxl: remove ctx->waitpid_instead
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (12 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 13/20] libxl: Allow AO_GC and EGC_GC even if not used Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 15/20] libxl: change some structures to unit arrays Ian Jackson
` (5 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Remove this obsolete hook. Callers inside libxl which create and reap
children should use the mechanisms provided by the event system.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_exec.c | 12 +++---------
tools/libxl/libxl_internal.h | 3 ---
2 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index b10e79f..2ee2154 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -19,11 +19,6 @@
#include "libxl_internal.h"
-static int call_waitpid(pid_t (*waitpid_cb)(pid_t, int *, int), pid_t pid, int *status, int options)
-{
- return (waitpid_cb) ? waitpid_cb(pid, status, options) : waitpid(pid, status, options);
-}
-
static void check_open_fds(const char *what)
{
const char *env_debug;
@@ -344,7 +339,7 @@ int libxl__spawn_spawn(libxl__gc *gc,
if (!for_spawn) _exit(0); /* just detach then */
- got = call_waitpid(ctx->waitpid_instead, child, &status, 0);
+ got = waitpid(child, &status, 0);
assert(got == child);
rc = (WIFEXITED(status) ? WEXITSTATUS(status) :
@@ -404,7 +399,7 @@ int libxl__spawn_detach(libxl__gc *gc,
(unsigned long)for_spawn->intermediate);
abort(); /* things are very wrong */
}
- got = call_waitpid(ctx->waitpid_instead, for_spawn->intermediate, &status, 0);
+ got = waitpid(for_spawn->intermediate, &status, 0);
assert(got == for_spawn->intermediate);
if (!(WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL)) {
report_spawn_intermediate_status(gc, for_spawn, status);
@@ -421,14 +416,13 @@ int libxl__spawn_detach(libxl__gc *gc,
int libxl__spawn_check(libxl__gc *gc, libxl__spawn_starting *for_spawn)
{
- libxl_ctx *ctx = libxl__gc_owner(gc);
pid_t got;
int status;
if (!for_spawn) return 0;
assert(for_spawn->intermediate);
- got = call_waitpid(ctx->waitpid_instead, for_spawn->intermediate, &status, WNOHANG);
+ got = waitpid(for_spawn->intermediate, &status, WNOHANG);
if (!got) return 0;
assert(got == for_spawn->intermediate);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 74dc2c5..ae71f70 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -334,9 +334,6 @@ struct libxl__ctx {
int sigchld_selfpipe[2]; /* [0]==-1 means handler not installed */
LIBXL_LIST_HEAD(, libxl__ev_child) children;
- /* This is obsolete and must be removed: */
- int (*waitpid_instead)(pid_t pid, int *status, int flags);
-
libxl_version_info version_info;
};
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 15/20] libxl: change some structures to unit arrays
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (13 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 14/20] libxl: remove ctx->waitpid_instead Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 16/20] libxl: ao: convert libxl__spawn_* Ian Jackson
` (4 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
In the next patch these variables will turn into actual pointers.
To clarify that patch, prepare the ground by changing these variables
from "struct foo var" to "struct foo var[1]". This enables accesses
to them and their members to be made as if they were pointers.
No functional change.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_create.c | 18 +++++-----
tools/libxl/libxl_dm.c | 84 ++++++++++++++++++++++----------------------
2 files changed, 51 insertions(+), 51 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index dbc3cf0..8408c26 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -543,7 +543,7 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
{
libxl_ctx *ctx = libxl__gc_owner(gc);
libxl__spawner_starting *dm_starting = 0;
- libxl__domain_build_state state;
+ libxl__domain_build_state state[1];
uint32_t domid;
int i, ret;
@@ -585,12 +585,12 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
}
}
- memset(&state, 0, sizeof(state));
+ memset(state, 0, sizeof(*state));
if ( restore_fd >= 0 ) {
- ret = domain_restore(gc, &d_config->b_info, domid, restore_fd, &state);
+ ret = domain_restore(gc, &d_config->b_info, domid, restore_fd, state);
} else {
- ret = libxl__domain_build(gc, &d_config->b_info, domid, &state);
+ ret = libxl__domain_build(gc, &d_config->b_info, domid, state);
}
if (ret) {
@@ -628,7 +628,7 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
ret = init_console_info(&console, 0);
if ( ret )
goto error_out;
- libxl__device_console_add(gc, domid, &console, &state);
+ libxl__device_console_add(gc, domid, &console, state);
libxl__device_console_dispose(&console);
libxl_device_vkb_init(&vkb);
@@ -636,7 +636,7 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
libxl_device_vkb_dispose(&vkb);
ret = libxl__create_device_model(gc, domid, d_config,
- &state, &dm_starting);
+ state, &dm_starting);
if (ret < 0) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
"failed to create device model: %d", ret);
@@ -665,11 +665,11 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
if (need_qemu)
console.consback = LIBXL__CONSOLE_BACKEND_IOEMU;
- libxl__device_console_add(gc, domid, &console, &state);
+ libxl__device_console_add(gc, domid, &console, state);
libxl__device_console_dispose(&console);
if (need_qemu) {
- libxl__create_xenpv_qemu(gc, domid, d_config, &state, &dm_starting);
+ libxl__create_xenpv_qemu(gc, domid, d_config, state, &dm_starting);
}
break;
}
@@ -683,7 +683,7 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
== LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
libxl__qmp_initializations(gc, domid, d_config);
}
- ret = libxl__confirm_device_model_startup(gc, &state, dm_starting);
+ ret = libxl__confirm_device_model_startup(gc, state, dm_starting);
if (ret < 0) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
"device model did not start: %d", ret);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 7bf653a..3921e2a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -676,10 +676,10 @@ static int libxl__create_stubdom(libxl__gc *gc,
libxl_ctx *ctx = libxl__gc_owner(gc);
int i, num_console = STUBDOM_SPECIAL_CONSOLES, ret;
libxl__device_console *console;
- libxl_domain_config dm_config;
+ libxl_domain_config dm_config[1];
libxl_device_vfb vfb;
libxl_device_vkb vkb;
- libxl__domain_build_state stubdom_state;
+ libxl__domain_build_state stubdom_state[1];
uint32_t dm_domid;
char **args;
struct xs_permissions perm[2];
@@ -692,58 +692,58 @@ static int libxl__create_stubdom(libxl__gc *gc,
goto out;
}
- libxl_domain_create_info_init(&dm_config.c_info);
- dm_config.c_info.type = LIBXL_DOMAIN_TYPE_PV;
- dm_config.c_info.name = libxl__sprintf(gc, "%s-dm",
+ libxl_domain_create_info_init(&dm_config->c_info);
+ dm_config->c_info.type = LIBXL_DOMAIN_TYPE_PV;
+ dm_config->c_info.name = libxl__sprintf(gc, "%s-dm",
libxl__domid_to_name(gc, guest_domid));
- dm_config.c_info.ssidref = guest_config->b_info.device_model_ssidref;
+ dm_config->c_info.ssidref = guest_config->b_info.device_model_ssidref;
- libxl_uuid_generate(&dm_config.c_info.uuid);
+ libxl_uuid_generate(&dm_config->c_info.uuid);
- libxl_domain_build_info_init(&dm_config.b_info);
- libxl_domain_build_info_init_type(&dm_config.b_info, LIBXL_DOMAIN_TYPE_PV);
+ libxl_domain_build_info_init(&dm_config->b_info);
+ libxl_domain_build_info_init_type(&dm_config->b_info, LIBXL_DOMAIN_TYPE_PV);
- dm_config.b_info.max_vcpus = 1;
- dm_config.b_info.max_memkb = 32 * 1024;
- dm_config.b_info.target_memkb = dm_config.b_info.max_memkb;
+ dm_config->b_info.max_vcpus = 1;
+ dm_config->b_info.max_memkb = 32 * 1024;
+ dm_config->b_info.target_memkb = dm_config->b_info.max_memkb;
- dm_config.b_info.u.pv.kernel.path = libxl__abs_path(gc, "ioemu-stubdom.gz",
+ dm_config->b_info.u.pv.kernel.path = libxl__abs_path(gc, "ioemu-stubdom.gz",
libxl_xenfirmwaredir_path());
- dm_config.b_info.u.pv.cmdline = libxl__sprintf(gc, " -d %d", guest_domid);
- dm_config.b_info.u.pv.ramdisk.path = "";
- dm_config.b_info.u.pv.features = "";
+ dm_config->b_info.u.pv.cmdline = libxl__sprintf(gc, " -d %d", guest_domid);
+ dm_config->b_info.u.pv.ramdisk.path = "";
+ dm_config->b_info.u.pv.features = "";
- dm_config.b_info.device_model_version =
+ dm_config->b_info.device_model_version =
guest_config->b_info.device_model_version;
- dm_config.b_info.device_model =
+ dm_config->b_info.device_model =
guest_config->b_info.device_model;
- dm_config.b_info.extra = guest_config->b_info.extra;
- dm_config.b_info.extra_pv = guest_config->b_info.extra_pv;
- dm_config.b_info.extra_hvm = guest_config->b_info.extra_hvm;
+ dm_config->b_info.extra = guest_config->b_info.extra;
+ dm_config->b_info.extra_pv = guest_config->b_info.extra_pv;
+ dm_config->b_info.extra_hvm = guest_config->b_info.extra_hvm;
- dm_config.disks = guest_config->disks;
- dm_config.num_disks = guest_config->num_disks;
+ dm_config->disks = guest_config->disks;
+ dm_config->num_disks = guest_config->num_disks;
- dm_config.vifs = guest_config->vifs;
- dm_config.num_vifs = guest_config->num_vifs;
+ dm_config->vifs = guest_config->vifs;
+ dm_config->num_vifs = guest_config->num_vifs;
- ret = libxl__domain_create_info_setdefault(gc, &dm_config.c_info);
+ ret = libxl__domain_create_info_setdefault(gc, &dm_config->c_info);
if (ret) goto out;
- ret = libxl__domain_build_info_setdefault(gc, &dm_config.b_info);
+ ret = libxl__domain_build_info_setdefault(gc, &dm_config->b_info);
if (ret) goto out;
libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, &vfb, &vkb);
- dm_config.vfbs = &vfb;
- dm_config.num_vfbs = 1;
- dm_config.vkbs = &vkb;
- dm_config.num_vkbs = 1;
+ dm_config->vfbs = &vfb;
+ dm_config->num_vfbs = 1;
+ dm_config->vkbs = &vkb;
+ dm_config->num_vkbs = 1;
/* fixme: this function can leak the stubdom if it fails */
dm_domid = 0;
- ret = libxl__domain_make(gc, &dm_config.c_info, &dm_domid);
+ ret = libxl__domain_make(gc, &dm_config->c_info, &dm_domid);
if (ret)
goto out;
- ret = libxl__domain_build(gc, &dm_config.b_info, dm_domid, &stubdom_state);
+ ret = libxl__domain_build(gc, &dm_config->b_info, dm_domid, stubdom_state);
if (ret)
goto out;
@@ -788,20 +788,20 @@ retry_transaction:
if (errno == EAGAIN)
goto retry_transaction;
- for (i = 0; i < dm_config.num_disks; i++) {
- ret = libxl_device_disk_add(ctx, dm_domid, &dm_config.disks[i]);
+ for (i = 0; i < dm_config->num_disks; i++) {
+ ret = libxl_device_disk_add(ctx, dm_domid, &dm_config->disks[i]);
if (ret)
goto out_free;
}
- for (i = 0; i < dm_config.num_vifs; i++) {
- ret = libxl_device_nic_add(ctx, dm_domid, &dm_config.vifs[i]);
+ for (i = 0; i < dm_config->num_vifs; i++) {
+ ret = libxl_device_nic_add(ctx, dm_domid, &dm_config->vifs[i]);
if (ret)
goto out_free;
}
- ret = libxl_device_vfb_add(ctx, dm_domid, &dm_config.vfbs[0]);
+ ret = libxl_device_vfb_add(ctx, dm_domid, &dm_config->vfbs[0]);
if (ret)
goto out_free;
- ret = libxl_device_vkb_add(ctx, dm_domid, &dm_config.vkbs[0]);
+ ret = libxl_device_vkb_add(ctx, dm_domid, &dm_config->vkbs[0]);
if (ret)
goto out_free;
@@ -845,14 +845,14 @@ retry_transaction:
break;
}
ret = libxl__device_console_add(gc, dm_domid, &console[i],
- i == STUBDOM_CONSOLE_LOGGING ? &stubdom_state : NULL);
+ i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL);
if (ret)
goto out_free;
}
if (libxl__create_xenpv_qemu(gc, dm_domid,
- &dm_config,
- &stubdom_state,
+ dm_config,
+ stubdom_state,
&dm_starting) < 0) {
ret = ERROR_FAIL;
goto out_free;
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 16/20] libxl: ao: convert libxl__spawn_*
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (14 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 15/20] libxl: change some structures to unit arrays Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 17/20] libxl: make libxl_create run bootloader via callback Ian Jackson
` (3 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
libxl__spawn_spawn becomes a callback-style asynchronous function.
The implementation is now in terms of libxl__ev_* including
libxl_ev_child.
All the callers need to be updated. This includes the device model
spawning functions libxl__create_device_model and
libxl__create_stubdom; these are replaced with libxl__spawn_local_dm
and libxl__spawn_stubdom. libxl__confirm_device_model_startup is
abolished; instead the dm spawner calls back.
(The choice of which kind of device model to create is lifted out of
what used to be libxl__create_device_model, because that function was
indirectly recursive. Recursive callback-style operations are clumsy
because they require a pointer indirection for the nested states.)
Waiting for proper device model startup it is no longer notionally
optional. Previously the code appeared to tolerate this by passing
NULL for various libxl__spawner_starting* parameters to device model
spawners. However, this was not used anywhere.
Conversely, the "for_spawn" parameter to libxl__wait_for_offspring is
no longer supported. It remains as an unused formal parameter to
avoid updating, in this patch, all the call sites which pass NULL.
libxl__wait_for_offspring is in any case itself an obsolete function,
so this wrinkle will go away when its callers are updated to use the
event system. Consequently libxl__spawn_check is also abolished.
The "console ready" callback also remains unchanged in this patch.
The API for this needs to be reviewed in the context of the event
series and its reentrancy restrictions documented.
Thus their callers need to be updated. These are the domain creation
functions libxl_domain_create_new and _restore. These functions now
take ao_hows, and have a private state structure.
However domain creation remains not completely converted to the event
mechanism; in particular it runs the outward-facing function
libxl_run_bootloader with a NULL ao_how, which is quite wrong. As it
happens in the current code this is not a bug because none of the rest
of the functionality surrounding the bootloader call will mind if the
event loop is reentered in the middle of its execution.
The file-scope function libxl__set_fd_flag which was used by the
previous spawn arrangements becomes unused and is removed; other
places in libxl can use libxl_fd_set_nonblock and
libxl_fd_set_cloexec, which of course remain.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl.h | 14 ++-
tools/libxl/libxl_create.c | 198 +++++++++++++++++++-----
tools/libxl/libxl_dm.c | 219 +++++++++++++++-----------
tools/libxl/libxl_exec.c | 354 +++++++++++++++++++++---------------------
tools/libxl/libxl_internal.h | 286 +++++++++++++++++++++++-----------
tools/libxl/xl_cmdimpl.c | 6 +-
6 files changed, 677 insertions(+), 400 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 477b72a..6f59364 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -465,8 +465,18 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
/* domain related functions */
typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
-int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid);
-int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd);
+ /* fixme-ao Need to review this API. If we keep it, the reentrancy
+ * properties need to be documented but they may turn out to be too
+ * awkward */
+
+int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
+ libxl_console_ready cb, void *priv, uint32_t *domid,
+ const libxl_asyncop_how *ao_how);
+int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
+ libxl_console_ready cb, void *priv,
+ uint32_t *domid, int restore_fd,
+ const libxl_asyncop_how *ao_how);
+
void libxl_domain_config_dispose(libxl_domain_config *d_config);
int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info,
uint32_t domid, int fd);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 8408c26..09a03a7 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -537,16 +537,40 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
libxl_device_model_version_to_string(b_info->device_model_version));
}
-static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
- libxl_console_ready cb, void *priv,
- uint32_t *domid_out, int restore_fd)
+/*----- main domain creation -----*/
+
+/* We have a linear control flow; only one event callback is
+ * outstanding at any time. Each initiation and callback function
+ * arranges for the next to be called, as the very last thing it
+ * does. (If that particular sub-operation is not needed, a
+ * function will call the next event callback directly.)
+ */
+
+/* Event callbacks, in this order: */
+static void domcreate_devmodel_started(libxl__egc *egc,
+ libxl__dm_spawn_state *dmss,
+ int rc);
+
+/* Our own function to clean up and call the user's callback.
+ * The final call in the sequence. */
+static void domcreate_complete(libxl__egc *egc,
+ libxl__domain_create_state *dcs,
+ int rc);
+
+static void initiate_domain_create(libxl__egc *egc,
+ libxl__domain_create_state *dcs)
{
+ STATE_AO_GC(dcs->ao);
libxl_ctx *ctx = libxl__gc_owner(gc);
- libxl__spawner_starting *dm_starting = 0;
- libxl__domain_build_state state[1];
uint32_t domid;
int i, ret;
+ /* convenience aliases */
+ libxl_domain_config *d_config = dcs->guest_config;
+ int restore_fd = dcs->restore_fd;
+ libxl_console_ready cb = dcs->console_cb;
+ void *priv = dcs->console_cb_priv;
+
domid = 0;
ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
@@ -559,9 +583,12 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
goto error_out;
}
+ dcs->guest_domid = domid;
+ dcs->dmss.guest_domid = 0; /* means we haven't spawned */
+
if ( d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV && cb ) {
- if ( (*cb)(ctx, domid, priv) )
- goto error_out;
+ ret = (*cb)(ctx, domid, priv);
+ if (ret) goto error_out;
}
ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
@@ -585,7 +612,12 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
}
}
- memset(state, 0, sizeof(*state));
+ memset(&dcs->build_state, 0, sizeof(dcs->build_state));
+ libxl__domain_build_state *state = &dcs->build_state;
+ dcs->dmss.spawn.ao = ao;
+ dcs->dmss.guest_config = dcs->guest_config;
+ dcs->dmss.build_state = &dcs->build_state;
+ dcs->dmss.callback = domcreate_devmodel_started;
if ( restore_fd >= 0 ) {
ret = domain_restore(gc, &d_config->b_info, domid, restore_fd, state);
@@ -635,13 +667,13 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
libxl_device_vkb_add(ctx, domid, &vkb);
libxl_device_vkb_dispose(&vkb);
- ret = libxl__create_device_model(gc, domid, d_config,
- state, &dm_starting);
- if (ret < 0) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "failed to create device model: %d", ret);
- goto error_out;
- }
+ dcs->dmss.guest_domid = domid;
+ if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
+ libxl__spawn_stubdom(egc, &dcs->sdss);
+ else
+ libxl__spawn_local_dm(egc, &dcs->dmss);
+ return;
+
break;
}
case LIBXL_DOMAIN_TYPE_PV:
@@ -669,7 +701,9 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
libxl__device_console_dispose(&console);
if (need_qemu) {
- libxl__create_xenpv_qemu(gc, domid, d_config, state, &dm_starting);
+ dcs->dmss.guest_domid = domid;
+ libxl__spawn_local_dm(egc, &dcs->dmss);
+ return;
}
break;
}
@@ -678,17 +712,41 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
goto error_out;
}
- if (dm_starting) {
+ assert(!dcs->dmss.guest_domid);
+ domcreate_devmodel_started(egc, &dcs->dmss, 0);
+ return;
+
+ error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
+static void domcreate_devmodel_started(libxl__egc *egc,
+ libxl__dm_spawn_state *dmss,
+ int ret)
+{
+ libxl__domain_create_state *dcs = CONTAINER_OF(dmss, *dcs, dmss);
+ STATE_AO_GC(dmss->spawn.ao);
+ int i;
+ libxl_ctx *ctx = CTX;
+ int domid = dcs->guest_domid;
+
+ /* convenience aliases */
+ libxl_domain_config *d_config = dcs->guest_config;
+ libxl_console_ready cb = dcs->console_cb;
+ void *priv = dcs->console_cb_priv;
+
+ if (ret) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "device model did not start: %d", ret);
+ goto error_out;
+ }
+
+ if (dcs->dmss.guest_domid) {
if (d_config->b_info.device_model_version
== LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
libxl__qmp_initializations(gc, domid, d_config);
}
- ret = libxl__confirm_device_model_startup(gc, state, dm_starting);
- if (ret < 0) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "device model did not start: %d", ret);
- goto error_out;
- }
}
for (i = 0; i < d_config->num_pcidevs; i++)
@@ -716,38 +774,94 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
if ( cb && (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM ||
(d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
d_config->b_info.u.pv.bootloader ))) {
- if ( (*cb)(ctx, domid, priv) )
- goto error_out;
+ ret = (*cb)(ctx, domid, priv);
+ if (ret) goto error_out;
}
- *domid_out = domid;
- return 0;
+ domcreate_complete(egc, dcs, 0);
+ return;
error_out:
- if (domid)
- libxl_domain_destroy(ctx, domid);
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
- return ret;
+static void domcreate_complete(libxl__egc *egc,
+ libxl__domain_create_state *dcs,
+ int rc) {
+ STATE_AO_GC(dcs->ao);
+
+ if (rc) {
+ if (dcs->guest_domid) {
+ int rc2 = libxl_domain_destroy(CTX, dcs->guest_domid);
+ if (rc2)
+ LOG(ERROR, "unable to destroy domain %d following"
+ " failed creation", dcs->guest_domid);
+ }
+ dcs->guest_domid = -1;
+ }
+ dcs->callback(egc, dcs, rc, dcs->guest_domid);
+}
+
+/*----- application-facing domain creation interface -----*/
+
+typedef struct {
+ libxl__domain_create_state dcs;
+ uint32_t *domid_out;
+} libxl__app_domain_create_state;
+
+static void domain_create_cb(libxl__egc *egc,
+ libxl__domain_create_state *dcs,
+ int rc, uint32_t domid);
+
+static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
+ libxl_console_ready cb, void *priv, uint32_t *domid,
+ int restore_fd, const libxl_asyncop_how *ao_how)
+{
+ AO_CREATE(ctx, 0, ao_how);
+ libxl__app_domain_create_state *cdcs;
+
+ GCNEW(cdcs);
+ cdcs->dcs.ao = ao;
+ cdcs->dcs.guest_config = d_config;
+ cdcs->dcs.restore_fd = restore_fd;
+ cdcs->dcs.console_cb = cb;
+ cdcs->dcs.console_cb_priv = priv;
+ cdcs->dcs.callback = domain_create_cb;
+ cdcs->domid_out = domid;
+
+ initiate_domain_create(egc, &cdcs->dcs);
+
+ return AO_INPROGRESS;
}
+static void domain_create_cb(libxl__egc *egc,
+ libxl__domain_create_state *dcs,
+ int rc, uint32_t domid)
+{
+ libxl__app_domain_create_state *cdcs = CONTAINER_OF(dcs, *cdcs, dcs);
+ STATE_AO_GC(cdcs->dcs.ao);
+
+ if (!rc)
+ *cdcs->domid_out = domid;
+
+ libxl__ao_complete(egc, ao, rc);
+}
+
int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
- libxl_console_ready cb, void *priv, uint32_t *domid)
+ libxl_console_ready cb, void *priv,
+ uint32_t *domid,
+ const libxl_asyncop_how *ao_how)
{
- GC_INIT(ctx);
- int rc;
- rc = do_domain_create(gc, d_config, cb, priv, domid, -1);
- GC_FREE;
- return rc;
+ return do_domain_create(ctx, d_config, cb, priv, domid, -1, ao_how);
}
int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
- libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd)
+ libxl_console_ready cb, void *priv,
+ uint32_t *domid, int restore_fd,
+ const libxl_asyncop_how *ao_how)
{
- GC_INIT(ctx);
- int rc;
- rc = do_domain_create(gc, d_config, cb, priv, domid, restore_fd);
- GC_FREE;
- return rc;
+ return do_domain_create(ctx, d_config, cb, priv, domid, restore_fd, ao_how);
}
/*
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3921e2a..15472a8 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -667,24 +667,28 @@ retry_transaction:
return 0;
}
-static int libxl__create_stubdom(libxl__gc *gc,
- int guest_domid,
- libxl_domain_config *guest_config,
- libxl__domain_build_state *d_state,
- libxl__spawner_starting **starting_r)
+static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
+ libxl__dm_spawn_state *stubdom_dmss,
+ int rc);
+
+void libxl__spawn_stubdom(libxl__egc *egc, libxl__stubdom_spawn_state *sdss)
{
+ STATE_AO_GC(sdss->stubdom.spawn.ao);
libxl_ctx *ctx = libxl__gc_owner(gc);
int i, num_console = STUBDOM_SPECIAL_CONSOLES, ret;
libxl__device_console *console;
- libxl_domain_config dm_config[1];
libxl_device_vfb vfb;
libxl_device_vkb vkb;
- libxl__domain_build_state stubdom_state[1];
- uint32_t dm_domid;
char **args;
struct xs_permissions perm[2];
xs_transaction_t t;
- libxl__spawner_starting *dm_starting = 0;
+
+ /* convenience aliases */
+ libxl_domain_config *dm_config = &sdss->stubdom_config;
+ libxl_domain_config *guest_config = sdss->stubdom.guest_config;
+ int guest_domid = sdss->stubdom.guest_domid;
+ libxl__domain_build_state *d_state = sdss->stubdom.build_state;
+ libxl__domain_build_state *stubdom_state = &sdss->stubdom_state;
if (guest_config->b_info.device_model_version !=
LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
@@ -692,6 +696,8 @@ static int libxl__create_stubdom(libxl__gc *gc,
goto out;
}
+ sdss->pvqemu.guest_domid = 0;
+
libxl_domain_create_info_init(&dm_config->c_info);
dm_config->c_info.type = LIBXL_DOMAIN_TYPE_PV;
dm_config->c_info.name = libxl__sprintf(gc, "%s-dm",
@@ -739,10 +745,10 @@ static int libxl__create_stubdom(libxl__gc *gc,
dm_config->num_vkbs = 1;
/* fixme: this function can leak the stubdom if it fails */
- dm_domid = 0;
- ret = libxl__domain_make(gc, &dm_config->c_info, &dm_domid);
+ ret = libxl__domain_make(gc, &dm_config->c_info, &sdss->pvqemu.guest_domid);
if (ret)
goto out;
+ uint32_t dm_domid = sdss->pvqemu.guest_domid;
ret = libxl__domain_build(gc, &dm_config->b_info, dm_domid, stubdom_state);
if (ret)
goto out;
@@ -850,42 +856,67 @@ retry_transaction:
goto out_free;
}
- if (libxl__create_xenpv_qemu(gc, dm_domid,
- dm_config,
- stubdom_state,
- &dm_starting) < 0) {
- ret = ERROR_FAIL;
- goto out_free;
- }
- if (libxl__confirm_device_model_startup(gc, d_state, dm_starting) < 0) {
- ret = ERROR_FAIL;
- goto out_free;
- }
-
- libxl_domain_unpause(ctx, dm_domid);
+ sdss->pvqemu.guest_domid = dm_domid;
+ sdss->pvqemu.guest_config = &sdss->stubdom_config;
+ sdss->pvqemu.build_state = &sdss->stubdom_state;
+ sdss->pvqemu.callback = spawn_stubdom_pvqemu_cb;
- if (starting_r) {
- *starting_r = calloc(1, sizeof(libxl__spawner_starting));
- (*starting_r)->domid = guest_domid;
- (*starting_r)->dom_path = libxl__xs_get_dompath(gc, guest_domid);
- (*starting_r)->for_spawn = NULL;
- }
+ libxl__spawn_local_dm(egc, &sdss->pvqemu);
- ret = 0;
+ free(args);
+ return;
out_free:
free(args);
out:
- return ret;
+ assert(ret);
+ spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, ret);
}
-int libxl__create_device_model(libxl__gc *gc,
- int domid,
- libxl_domain_config *guest_config,
- libxl__domain_build_state *state,
- libxl__spawner_starting **starting_r)
+static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
+ libxl__dm_spawn_state *stubdom_dmss,
+ int rc)
{
- libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl__stubdom_spawn_state *sdss =
+ CONTAINER_OF(stubdom_dmss, *sdss, pvqemu);
+ STATE_AO_GC(sdss->stubdom.spawn.ao);
+ uint32_t dm_domid = sdss->pvqemu.guest_domid;
+
+ if (rc) goto out;
+
+ rc = libxl_domain_unpause(CTX, dm_domid);
+ if (rc) goto out;
+
+ out:
+ if (rc) {
+ if (dm_domid)
+ libxl_domain_destroy(CTX, dm_domid);
+ }
+ sdss->callback(egc, &sdss->stubdom, rc);
+}
+
+/* callbacks passed to libxl__spawn_spawn */
+static void device_model_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
+ const char *xsdata);
+static void device_model_startup_failed(libxl__egc *egc,
+ libxl__spawn_state *spawn);
+
+/* our "next step" function, called from those callbacks and elsewhere */
+static void device_model_spawn_outcome(libxl__egc *egc,
+ libxl__dm_spawn_state *dmss,
+ int rc);
+
+void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
+{
+ /* convenience aliases */
+ int domid = dmss->guest_domid;
+ libxl__domain_build_state *state = dmss->build_state;
+ libxl__spawn_state *spawn = &dmss->spawn;
+
+ STATE_AO_GC(dmss->spawn.ao);
+
+ libxl_ctx *ctx = CTX;
+ libxl_domain_config *guest_config = dmss->guest_config;
const libxl_domain_create_info *c_info = &guest_config->c_info;
const libxl_domain_build_info *b_info = &guest_config->b_info;
const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
@@ -893,15 +924,13 @@ int libxl__create_device_model(libxl__gc *gc,
int logfile_w, null;
int rc;
char **args, **arg;
- libxl__spawner_starting buf_starting, *p;
xs_transaction_t t;
char *vm_path;
char **pass_stuff;
const char *dm;
if (libxl_defbool_val(b_info->device_model_stubdomain)) {
- rc = libxl__create_stubdom(gc, domid, guest_config, state, starting_r);
- goto out;
+ abort();
}
dm = libxl__domain_device_model(gc, b_info);
@@ -945,25 +974,8 @@ int libxl__create_device_model(libxl__gc *gc,
free(logfile);
null = open("/dev/null", O_RDONLY);
- if (starting_r) {
- rc = ERROR_NOMEM;
- *starting_r = calloc(1, sizeof(libxl__spawner_starting));
- if (!*starting_r)
- goto out_close;
- p = *starting_r;
- p->for_spawn = calloc(1, sizeof(libxl__spawn_starting));
- } else {
- p = &buf_starting;
- p->for_spawn = NULL;
- }
-
- p->domid = domid;
- p->dom_path = libxl__xs_get_dompath(gc, domid);
- p->pid_path = "image/device-model-pid";
- if (!p->dom_path) {
- rc = ERROR_FAIL;
- goto out_close;
- }
+ const char *dom_path = libxl__xs_get_dompath(gc, domid);
+ spawn->pidpath = GCSPRINTF("%s/%s", dom_path, "image/device-model-pid");
if (vnc && vnc->passwd) {
/* This xenstore key will only be used by qemu-xen-traditionnal.
@@ -971,7 +983,7 @@ int libxl__create_device_model(libxl__gc *gc,
retry_transaction:
/* Find uuid and the write the vnc password to xenstore for qemu. */
t = xs_transaction_start(ctx->xsh);
- vm_path = libxl__xs_read(gc,t,libxl__sprintf(gc, "%s/vm", p->dom_path));
+ vm_path = libxl__xs_read(gc,t,libxl__sprintf(gc, "%s/vm", dom_path));
if (vm_path) {
/* Now write the vncpassword into it. */
pass_stuff = libxl__calloc(gc, 3, sizeof(char *));
@@ -988,8 +1000,15 @@ retry_transaction:
for (arg = args; *arg; arg++)
LIBXL__LOG(CTX, XTL_DEBUG, " %s", *arg);
- rc = libxl__spawn_spawn(gc, p->for_spawn, "device model",
- libxl_spawner_record_pid, p);
+ spawn->what = GCSPRINTF("domain %d device model", domid);
+ spawn->xspath = GCSPRINTF("/local/domain/0/device-model/%d/state", domid);
+ spawn->timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
+ spawn->pidpath = GCSPRINTF("%s/image/device-model-pid", dom_path);
+ spawn->midproc_cb = libxl__spawn_record_pid;
+ spawn->confirm_cb = device_model_confirm;
+ spawn->failure_cb = device_model_startup_failed;
+
+ rc = libxl__spawn_spawn(egc, spawn);
if (rc < 0)
goto out_close;
if (!rc) { /* inner child */
@@ -1004,30 +1023,61 @@ out_close:
close(logfile_w);
free(args);
out:
- return rc;
+ if (rc)
+ device_model_spawn_outcome(egc, dmss, rc);
+}
+
+
+static void device_model_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
+ const char *xsdata)
+{
+ libxl__dm_spawn_state *dmss = CONTAINER_OF(spawn, *dmss, spawn);
+ STATE_AO_GC(spawn->ao);
+
+ if (!xsdata)
+ return;
+
+ if (strcmp(xsdata, "running"))
+ return;
+
+ libxl__spawn_detach(gc, spawn);
+
+ device_model_spawn_outcome(egc, dmss, 0);
}
+static void device_model_startup_failed(libxl__egc *egc,
+ libxl__spawn_state *spawn)
+{
+ libxl__dm_spawn_state *dmss = CONTAINER_OF(spawn, *dmss, spawn);
+ device_model_spawn_outcome(egc, dmss, ERROR_FAIL);
+}
-int libxl__confirm_device_model_startup(libxl__gc *gc,
- libxl__domain_build_state *state,
- libxl__spawner_starting *starting)
+static void device_model_spawn_outcome(libxl__egc *egc,
+ libxl__dm_spawn_state *dmss,
+ int rc)
{
- char *path;
- int domid = starting->domid;
- int ret, ret2;
- path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
- ret = libxl__spawn_confirm_offspring_startup(gc,
- LIBXL_DEVICE_MODEL_START_TIMEOUT,
- "Device Model", path, "running", starting);
+ STATE_AO_GC(dmss->spawn.ao);
+ int ret2;
+
+ if (rc)
+ LOG(ERROR, "%s: spawn failed (rc=%d)", dmss->spawn.what, rc);
+
+ libxl__domain_build_state *state = dmss->build_state;
+
if (state->saved_state) {
ret2 = unlink(state->saved_state);
- if (ret2) LIBXL__LOG_ERRNO(CTX, XTL_ERROR,
- "failed to remove device-model state %s\n",
- state->saved_state);
- /* Do not clobber spawn_confirm error code with unlink error code. */
- if (!ret) ret = ret2;
+ if (ret2) {
+ LOGE(ERROR, "%s: failed to remove device-model state %s",
+ dmss->spawn.what, state->saved_state);
+ rc = ERROR_FAIL;
+ goto out;
+ }
}
- return ret;
+
+ rc = 0;
+
+ out:
+ dmss->callback(egc, dmss, rc);
}
int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
@@ -1123,15 +1173,6 @@ out:
return ret;
}
-int libxl__create_xenpv_qemu(libxl__gc *gc, uint32_t domid,
- libxl_domain_config *guest_config,
- libxl__domain_build_state *state,
- libxl__spawner_starting **starting_r)
-{
- libxl__create_device_model(gc, domid, guest_config, state, starting_r);
- return 0;
-}
-
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 2ee2154..d44b702 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -127,29 +127,35 @@ void libxl_report_child_exitstatus(libxl_ctx *ctx,
}
}
-void libxl_spawner_record_pid(void *for_spawn, pid_t innerchild)
+int libxl__spawn_record_pid(libxl__gc *gc, libxl__spawn_state *spawn,
+ pid_t innerchild)
{
- libxl__spawner_starting *starting = for_spawn;
- struct xs_handle *xsh;
- char *path = NULL, *pid = NULL;
- int len;
-
- if (asprintf(&path, "%s/%s", starting->dom_path, starting->pid_path) < 0)
- goto out;
+ struct xs_handle *xsh = NULL;
+ const char *pid = NULL;
+ int rc, xsok;
- len = asprintf(&pid, "%d", innerchild);
- if (len < 0)
- goto out;
+ pid = GCSPRINTF("%d", innerchild);
/* we mustn't use the parent's handle in the child */
xsh = xs_daemon_open();
+ if (!xsh) {
+ LOGE(ERROR, "write %s = %s: xenstore reopen failed",
+ spawn->pidpath, pid);
+ rc = ERROR_FAIL; goto out;
+ }
- xs_write(xsh, XBT_NULL, path, pid, len);
+ xsok = xs_write(xsh, XBT_NULL, spawn->pidpath, pid, strlen(pid));
+ if (!xsok) {
+ LOGE(ERROR,
+ "write %s = %s: xenstore write failed", spawn->pidpath, pid);
+ rc = ERROR_FAIL; goto out;
+ }
+
+ rc = 0;
- xs_daemon_close(xsh);
out:
- free(path);
- free(pid);
+ if (xsh) xs_daemon_close(xsh);
+ return rc ? SIGTERM : 0;
}
int libxl__wait_for_offspring(libxl__gc *gc,
@@ -184,19 +190,9 @@ int libxl__wait_for_offspring(libxl__gc *gc,
tv.tv_sec = timeout;
tv.tv_usec = 0;
nfds = xs_fileno(xsh) + 1;
- if (spawning && spawning->fd > xs_fileno(xsh))
- nfds = spawning->fd + 1;
+ assert(!spawning);
while (rc > 0 || (!rc && tv.tv_sec > 0)) {
- if ( spawning ) {
- rc = libxl__spawn_check(gc, spawning);
- if ( rc ) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "%s died during startup", what);
- rc = -1;
- goto err_died;
- }
- }
p = xs_read(xsh, XBT_NULL, path, &len);
if ( NULL == p )
goto again;
@@ -218,8 +214,6 @@ again:
free(p);
FD_ZERO(&rfds);
FD_SET(xs_fileno(xsh), &rfds);
- if (spawning)
- FD_SET(spawning->fd, &rfds);
rc = select(nfds, &rfds, NULL, NULL, &tv);
if (rc > 0) {
if (FD_ISSET(xs_fileno(xsh), &rfds)) {
@@ -229,207 +223,215 @@ again:
else
goto again;
}
- if (spawning && FD_ISSET(spawning->fd, &rfds)) {
- unsigned char dummy;
- if (read(spawning->fd, &dummy, sizeof(dummy)) != 1)
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_DEBUG,
- "failed to read spawn status pipe");
- }
}
}
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s not ready", what);
-err_died:
+
xs_unwatch(xsh, path, path);
xs_daemon_close(xsh);
err:
return -1;
}
-static int detach_offspring(libxl__gc *gc,
- libxl__spawner_starting *starting)
-{
- int rc;
- rc = libxl__spawn_detach(gc, starting->for_spawn);
- if (starting->for_spawn)
- free(starting->for_spawn);
- free(starting);
- return rc;
-}
-int libxl__spawn_confirm_offspring_startup(libxl__gc *gc,
- uint32_t timeout, char *what,
- char *path, char *state,
- libxl__spawner_starting *starting)
-{
- int detach;
- int problem = libxl__wait_for_offspring(gc, starting->domid, timeout, what,
- path, state,
- starting->for_spawn, NULL, NULL);
- detach = detach_offspring(gc, starting);
- return problem ? problem : detach;
-}
+/*----- spawn implementation -----*/
-static int libxl__set_fd_flag(libxl__gc *gc, int fd, int flag)
-{
- int flags;
+/*
+ * Full set of possible states of a libxl__spawn_state and its _detachable:
+ *
+ * ss-> ss-> ss-> | ssd-> ssd->
+ * timeout xswatch ssd | mid ss
+ * - Undefined undef undef no | - -
+ * - Idle Idle Idle no | - -
+ * - Active Active Active yes | Active yes
+ * - Partial Active/Idle Active/Idle maybe | Active/Idle yes (if exists)
+ * - Detached - - - | Active no
+ *
+ * When in state Detached, the middle process has been sent a SIGKILL.
+ */
- flags = fcntl(fd, F_GETFL);
- if (flags == -1)
- return ERROR_FAIL;
+/* 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_middle_death(libxl__egc *egc, libxl__ev_child *childw,
+ pid_t pid, int status);
- flags |= flag;
+/* Precondition: Partial. Results: Detached. */
+static void spawn_cleanup(libxl__gc *gc, libxl__spawn_state *ss);
- if (fcntl(fd, F_SETFL, flags) == -1)
- return ERROR_FAIL;
+/* Precondition: Partial; caller has logged failure reason.
+ * Results: Caller notified of failure;
+ * after return, ss may be completely invalid as caller may reuse it */
+static void spawn_failed(libxl__egc *egc, libxl__spawn_state *ss);
- return 0;
+void libxl__spawn_init(libxl__spawn_state *ss)
+{
+ libxl__ev_time_init(&ss->timeout);
+ libxl__ev_xswatch_init(&ss->xswatch);
+ ss->ssd = 0;
}
-int libxl__spawn_spawn(libxl__gc *gc,
- libxl__spawn_starting *for_spawn,
- const char *what,
- void (*intermediate_hook)(void *for_spawn,
- pid_t innerchild),
- void *hook_data)
+int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
{
- libxl_ctx *ctx = libxl__gc_owner(gc);
- pid_t child, got;
+ STATE_AO_GC(ss->ao);
+ int r;
+ pid_t child;
int status, rc;
- pid_t intermediate;
- int pipes[2];
- unsigned char dummy = 0;
-
- if (for_spawn) {
- for_spawn->what = strdup(what);
- if (!for_spawn->what) return ERROR_NOMEM;
-
- if (libxl_pipe(ctx, pipes) < 0)
- goto err_parent;
- if (libxl__set_fd_flag(gc, pipes[0], O_NONBLOCK) < 0 ||
- libxl__set_fd_flag(gc, pipes[1], O_NONBLOCK) < 0)
- goto err_parent_pipes;
- }
- intermediate = libxl_fork(ctx);
- if (intermediate ==-1)
- goto err_parent_pipes;
+ libxl__spawn_init(ss);
+ ss->ssd = libxl__zalloc(0, sizeof(*ss->ssd));
+ libxl__ev_child_init(&ss->ssd->mid);
+
+ rc = libxl__ev_time_register_rel(gc, &ss->timeout,
+ spawn_timeout, ss->timeout_ms);
+ if (rc) goto out_err;
- if (intermediate) {
+ rc = libxl__ev_xswatch_register(gc, &ss->xswatch,
+ spawn_watch_event, ss->xspath);
+ if (rc) goto out_err;
+
+ pid_t middle = libxl__ev_child_fork(gc, &ss->ssd->mid, spawn_middle_death);
+ if (middle ==-1) { rc = ERROR_FAIL; goto out_err; }
+
+ if (middle) {
/* parent */
- if (for_spawn) {
- for_spawn->intermediate = intermediate;
- for_spawn->fd = pipes[0];
- close(pipes[1]);
- }
return 1;
}
- /* we are now the intermediate process */
- if (for_spawn) close(pipes[0]);
+ /* we are now the middle process */
child = fork();
if (child == -1)
exit(255);
if (!child) {
- if (for_spawn) close(pipes[1]);
return 0; /* caller runs child code */
}
- intermediate_hook(hook_data, child);
-
- if (!for_spawn) _exit(0); /* just detach then */
-
- got = waitpid(child, &status, 0);
- assert(got == child);
-
- rc = (WIFEXITED(status) ? WEXITSTATUS(status) :
- WIFSIGNALED(status) && WTERMSIG(status) < 127
- ? WTERMSIG(status)+128 : -1);
- if (for_spawn) {
- if (write(pipes[1], &dummy, sizeof(dummy)) != 1)
- perror("libxl__spawn_spawn: unable to signal child exit to parent");
+ int failsig = ss->midproc_cb(gc, ss, middle);
+ if (failsig) {
+ kill(child, failsig);
+ _exit(127);
}
- _exit(rc);
- err_parent_pipes:
- if (for_spawn) {
- close(pipes[0]);
- close(pipes[1]);
+ for (;;) {
+ pid_t got = waitpid(child, &status, 0);
+ if (got == -1) {
+ assert(errno == EINTR);
+ continue;
+ }
+ assert(got == child);
+ break;
}
- err_parent:
- if (for_spawn) free(for_spawn->what);
+ r = (WIFEXITED(status) && WEXITSTATUS(status) <= 127 ? WEXITSTATUS(status) :
+ WIFSIGNALED(status) && WTERMSIG(status) < 127 ? WTERMSIG(status)+128 :
+ -1);
+ _exit(r);
- return ERROR_FAIL;
+ out_err:
+ spawn_cleanup(gc, ss);
+ return rc;
}
-static void report_spawn_intermediate_status(libxl__gc *gc,
- libxl__spawn_starting *for_spawn,
- int status)
+static void spawn_cleanup(libxl__gc *gc, libxl__spawn_state *ss)
{
- if (!WIFEXITED(status)) {
- libxl_ctx *ctx = libxl__gc_owner(gc);
- char *intermediate_what;
- /* intermediate process did the logging itself if it exited */
- if ( asprintf(&intermediate_what,
- "%s intermediate process (startup monitor)",
- for_spawn->what) < 0 )
- intermediate_what = "intermediate process (startup monitor)";
- libxl_report_child_exitstatus(ctx, LIBXL__LOG_ERROR, intermediate_what,
- for_spawn->intermediate, status);
+ int r;
+
+ libxl__ev_time_deregister(gc, &ss->timeout);
+ libxl__ev_xswatch_deregister(gc, &ss->xswatch);
+
+ libxl__spawn_state_detachable *ssd = ss->ssd;
+ if (ssd) {
+ if (libxl__ev_child_inuse(&ssd->mid)) {
+ pid_t child = ssd->mid.pid;
+ r = kill(child, SIGKILL);
+ if (r && errno != ESRCH)
+ LOGE(WARN, "%s: failed to kill intermediate child (pid=%lu)",
+ ss->what, (unsigned long)child);
+ }
+
+ /* disconnect the ss and ssd from each other */
+ ssd->ss = 0;
+ ss->ssd = 0;
}
}
-int libxl__spawn_detach(libxl__gc *gc,
- libxl__spawn_starting *for_spawn)
+static void spawn_failed(libxl__egc *egc, libxl__spawn_state *ss)
{
- libxl_ctx *ctx = libxl__gc_owner(gc);
- int r, status;
- pid_t got;
- int rc = 0;
+ EGC_GC;
+ spawn_cleanup(gc, ss);
+ ss->failure_cb(egc, ss); /* must be last; callback may do anything to ss */
+}
- if (!for_spawn) return 0;
+static void spawn_timeout(libxl__egc *egc, libxl__ev_time *ev,
+ const struct timeval *requested_abs)
+{
+ /* Before event, was Active; is now Partial. */
+ EGC_GC;
+ libxl__spawn_state *ss = CONTAINER_OF(ev, *ss, timeout);
+ LOG(ERROR, "%s: startup timed out", ss->what);
+ spawn_failed(egc, ss); /* must be last */
+}
- if (for_spawn->intermediate) {
- r = kill(for_spawn->intermediate, SIGKILL);
- if (r) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
- "could not kill %s intermediate process [%ld]",
- for_spawn->what,
- (unsigned long)for_spawn->intermediate);
- abort(); /* things are very wrong */
- }
- got = waitpid(for_spawn->intermediate, &status, 0);
- assert(got == for_spawn->intermediate);
- if (!(WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL)) {
- report_spawn_intermediate_status(gc, for_spawn, status);
- rc = ERROR_FAIL;
- }
- for_spawn->intermediate = 0;
+static void spawn_watch_event(libxl__egc *egc, libxl__ev_xswatch *xsw,
+ const char *watch_path, const char *event_path)
+{
+ /* On entry, is Active. */
+ 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);
+ spawn_failed(egc, ss); /* must be last */
+ return;
}
-
- free(for_spawn->what);
- for_spawn->what = 0;
-
- return rc;
+ ss->confirm_cb(egc, ss, p); /* must be last */
}
-int libxl__spawn_check(libxl__gc *gc, libxl__spawn_starting *for_spawn)
+static void spawn_middle_death(libxl__egc *egc, libxl__ev_child *childw,
+ pid_t pid, int status)
+ /* Before event, was Active or Detached;
+ * is now Active or Detached except that ssd->mid is Idle */
{
- pid_t got;
- int status;
-
- if (!for_spawn) return 0;
+ EGC_GC;
+ libxl__spawn_state_detachable *ssd = CONTAINER_OF(childw, *ssd, mid);
+ libxl__spawn_state *ss = ssd->ss;
- assert(for_spawn->intermediate);
- got = waitpid(for_spawn->intermediate, &status, WNOHANG);
- if (!got) return 0;
-
- assert(got == for_spawn->intermediate);
- report_spawn_intermediate_status(gc, for_spawn, status);
+ if (!WIFEXITED(status)) {
+ const char *what =
+ GCSPRINTF("%s intermediate process (startup monitor)",
+ ss ? ss->what : "(detached)");
+ int loglevel = ss ? XTL_ERROR : XTL_WARN;
+ libxl_report_child_exitstatus(CTX, loglevel, what, pid, status);
+ } else if (ss) { /* otherwise it was supposed to be a daemon by now */
+ if (!status)
+ LOG(ERROR, "%s [%ld]: unexpectedly exited with exit status 0,"
+ " when we were waiting for it to confirm startup",
+ ss->what, (unsigned long)pid);
+ else if (status <= 127)
+ LOG(ERROR, "%s [%ld]: failed startup with non-zero exit status %d",
+ ss->what, (unsigned long)pid, status);
+ else if (status < 255) {
+ int sig = status - 128;
+ const char *str = strsignal(sig);
+ if (str)
+ LOG(ERROR, "%s [%ld]: died during startup due to fatal"
+ " signal %s", ss->what, (unsigned long)pid, str);
+ else
+ LOG(ERROR, "%s [%ld]: died during startup due to unknown fatal"
+ " signal number %d", ss->what, (unsigned long)pid, sig);
+ }
+ ss->ssd = 0; /* detatch the ssd to make the ss be in state Partial */
+ spawn_failed(egc, ss); /* must be last use of ss */
+ }
+ free(ssd);
+}
- for_spawn->intermediate = 0;
- return ERROR_FAIL;
+void libxl__spawn_detach(libxl__gc *gc, libxl__spawn_state *ss)
+{
+ spawn_cleanup(gc, ss);
}
/*
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ae71f70..5bab4d6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -840,75 +840,197 @@ _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);
-/* xl_exec */
+/*
+ *----- spawn -----
+ *
+ * Higher-level double-fork and separate detach eg as for device models
+ *
+ * Each libxl__spawn_state is in one of the conventional states
+ * Undefined, Idle, Active
+ */
- /* higher-level double-fork and separate detach eg as for device models */
+typedef struct libxl__obsolete_spawn_starting libxl__spawn_starting;
+/* this type is never defined, so no objects of this type exist
+ * fixme-ao This should go away completely. */
-typedef struct {
- /* put this in your own status structure as returned to application */
- /* all fields are private to libxl_spawn_... */
- pid_t intermediate;
- int fd;
- char *what; /* malloc'd in spawn_spawn */
-} libxl__spawn_starting;
+typedef struct libxl__spawn_state libxl__spawn_state;
-typedef struct {
- char *dom_path; /* from libxl_malloc, only for libxl_spawner_record_pid */
- const char *pid_path; /* only for libxl_spawner_record_pid */
- int domid;
- libxl__spawn_starting *for_spawn;
-} libxl__spawner_starting;
+/* Clears out a spawn state; idempotent. */
+_hidden void libxl__spawn_init(libxl__spawn_state*);
/*
- * libxl__spawn_spawn - Create a new process
- * gc: allocation pool
- * for_spawn: malloc'd pointer to libxl__spawn_starting (optional)
- * what: string describing the spawned process
- * intermediate_hook: helper to record pid, such as libxl_spawner_record_pid
- * hook_data: data to pass to the hook function
+ * libxl__spawn_spawn - Create a new process which will become daemonic
+ * Forks twice, to allow the child to detach entirely from the parent.
+ *
+ * We call the two generated processes the "middle child" (result of
+ * the first fork) and the "inner child" (result of the second fork
+ * which takes place in the middle child).
+ *
+ * The inner child must soon exit or exec. If must also soon exit or
+ * notify the parent of its successful startup by writing to the
+ * xenstore path xspath (or by other means). xspath may be 0 to
+ * indicate that only other means are being used.
+ *
+ * The user (in the parent) will be called back (confirm_cb) every
+ * time that xenstore path is modified.
+ *
+ * In both children, the ctx is not fully useable: gc and logging
+ * operations are OK, but operations on Xen and xenstore are not.
+ * (The restrictions are the same as those which apply to children
+ * made with libxl__ev_child_fork.)
+ *
+ * midproc_cb will be called in the middle child, with the pid of the
+ * inner child; this could for example record the pid. midproc_cb
+ * should be fast, and should return. It will be called (reentrantly)
+ * within libxl__spawn_init.
+ *
+ * failure_cb will be called in the parent on failure of the
+ * intermediate or final child; an error message will have been
+ * logged.
+ *
+ * confirm_cb and failure_cb will not be called reentrantly from
+ * within libxl__spawn_spawn.
+ *
+ * what: string describing the spawned process, used for logging
*
* Logs errors. A copy of "what" is taken.
* Return values:
- * < 0 error, for_spawn need not be detached
- * +1 caller is the parent, must call detach on *for_spawn eventually
+ * < 0 error, *spawn is now Idle and need not be detached
+ * +1 caller is the parent, *spawn is Active and must eventually be detached
* 0 caller is now the inner child, should probably call libxl__exec
- * Caller, may pass 0 for for_spawn, in which case no need to detach.
+ *
+ * The spawn state must be Undefined or Idle on entry.
*/
-_hidden int libxl__spawn_spawn(libxl__gc *gc,
- libxl__spawn_starting *for_spawn,
- const char *what,
- void (*intermediate_hook)(void *for_spawn, pid_t innerchild),
- void *hook_data);
+_hidden int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *spawn);
/*
- * libxl_spawner_record_pid - Record given pid in xenstore
- * for_spawn: malloc'd pointer to libxl__spawn_starting (optional)
- * innerchild: pid of the child
+ * libxl__spawn_detach - Detaches the daemonic child.
+ *
+ * Works by killing the intermediate process from spawn_spawn.
+ * After this function returns, failures of either child are no
+ * longer repaorted via failure_cb.
+ *
+ * If called before the inner child has been created, this may prevent
+ * it from running at all. Thus this should be called only when the
+ * inner child has notified that it is ready. Normally it will be
+ * called from within confirm_cb.
*
- * This function is passed as intermediate_hook to libxl__spawn_spawn.
+ * Logs errors.
+ *
+ * The spawn state must be Active or Idle on entry and will be Idle
+ * on return.
*/
-_hidden void libxl_spawner_record_pid(void *for_spawn, pid_t innerchild);
+_hidden void libxl__spawn_detach(libxl__gc *gc, libxl__spawn_state*);
/*
- * libxl__spawn_confirm_offspring_startup - Wait for child state
- * gc: allocation pool
- * timeout: how many seconds to wait for the child
- * what: string describing the spawned process
- * path: path to the state file in xenstore
- * state: expected string to wait for in path (optional)
- * starting: malloc'd pointer to libxl__spawner_starting
+ * If successful, this should return 0.
*
- * Returns 0 on success, and < 0 on error.
+ * Otherwise it should return a signal number, which will be
+ * sent to the inner child; the overall spawn will then fail.
+ */
+typedef int /* signal number */
+libxl__spawn_midproc_cb(libxl__gc*, libxl__spawn_state*, pid_t inner);
+
+/*
+ * Called if the spawn failed. The reason will have been logged.
+ * The spawn state will be Idle on entry to the callback (and
+ * it may be reused immediately if desired).
+ */
+typedef void libxl__spawn_failure_cb(libxl__egc*, libxl__spawn_state*);
+
+/*
+ * Called when the xspath watch triggers. xspath will have been read
+ * and the result placed in xsdata; if that failed because the key
+ * didn't exist, xspath==0. (If it failed for some other reason,
+ * the spawn machinery calls failure_cb instead.)
*
- * This function waits the given timeout for the given path to appear
- * in xenstore, and optionally for state in path.
- * The intermediate process created in libxl__spawn_spawn is killed.
- * The memory referenced by starting->for_spawn and starting is free'd.
+ * If the child has indicated its successful startup, or a failure
+ * has occurred, this should call libxl__spawn_detach.
+ *
+ * If the child is still starting up, should simply return, doing
+ * nothing.
+ *
+ * The spawn state will be Active on entry to the callback; there
+ * are no restrictions on the state on return; it may even have
+ * been detached and reused.
+ */
+typedef void libxl__spawn_confirm_cb(libxl__egc*, libxl__spawn_state*,
+ const char *xsdata);
+
+typedef struct {
+ /* Private to the spawn implementation.
+ */
+ /* This separate struct, from malloc, allows us to "detach"
+ * the child and reap it later, when our user has gone
+ * away and freed its libxl__spawn_state */
+ struct libxl__spawn_state *ss;
+ libxl__ev_child mid;
+} libxl__spawn_state_detachable;
+
+struct libxl__spawn_state {
+ /* must be filled in by user and remain valid */
+ libxl__ao *ao;
+ const char *what;
+ const char *xspath;
+ const char *pidpath; /* only used by libxl__spawn_midproc_record_pid */
+ int timeout_ms; /* -1 means forever */
+ libxl__spawn_midproc_cb *midproc_cb;
+ libxl__spawn_failure_cb *failure_cb;
+ libxl__spawn_confirm_cb *confirm_cb;
+
+ /* remaining fields are private to libxl_spawn_... */
+ libxl__ev_time timeout;
+ libxl__ev_xswatch xswatch;
+ libxl__spawn_state_detachable *ssd;
+};
+
+static inline int libxl__spawn_inuse(libxl__spawn_state *ss)
+ { return !!ss->ssd; }
+
+/*
+ * libxl_spawner_record_pid - Record given pid in xenstore
+ *
+ * This function can be passed directly as an intermediate_hook to
+ * libxl__spawn_spawn. On failure, returns the value SIGTERM.
*/
-_hidden int libxl__spawn_confirm_offspring_startup(libxl__gc *gc,
- uint32_t timeout, char *what,
- char *path, char *state,
- libxl__spawner_starting *starting);
+_hidden int libxl__spawn_record_pid(libxl__gc*, libxl__spawn_state*,
+ pid_t innerchild);
+
+/*----- device model creation -----*/
+
+/* First layer; wraps libxl__spawn_spawn. */
+
+typedef struct libxl__dm_spawn_state libxl__dm_spawn_state;
+
+typedef void libxl__dm_spawn_cb(libxl__egc *egc, libxl__dm_spawn_state*,
+ int rc /* if !0, error was logged */);
+
+struct libxl__dm_spawn_state {
+ /* mixed - ao must be initialised by user; rest is private: */
+ libxl__spawn_state spawn;
+ /* filled in by user, must remain valid: */
+ uint32_t guest_domid; /* domain being served */
+ libxl_domain_config *guest_config;
+ libxl__domain_build_state *build_state; /* relates to guest_domid */
+ libxl__dm_spawn_cb *callback;
+};
+
+_hidden void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state*);
+
+/* Stubdoms. */
+
+typedef struct {
+ /* mixed - user must fill in public parts only: */
+ libxl__dm_spawn_state stubdom; /* will always remain the first member */
+ libxl__dm_spawn_cb *callback; /* called as callback(,&sdss->stubdom,) */
+ /* private to libxl__spawn_stubdom: */
+ libxl_domain_config stubdom_config;
+ libxl__domain_build_state stubdom_state;
+ libxl__dm_spawn_state pvqemu;
+} libxl__stubdom_spawn_state;
+
+_hidden void libxl__spawn_stubdom(libxl__egc *egc, libxl__stubdom_spawn_state*);
+
/*
* libxl__wait_for_offspring - Wait for child state
@@ -941,31 +1063,6 @@ _hidden int libxl__wait_for_offspring(libxl__gc *gc,
void *userdata),
void *check_callback_userdata);
-/*
- * libxl__spawn_detach - Kill intermediate process from spawn_spawn
- * gc: allocation pool
- * for_spawn: malloc'd pointer to libxl__spawn_starting (optional)
- *
- * Returns 0 on success, and < 0 on error.
- *
- * Logs errors. Idempotent, but only permitted after successful
- * call to libxl__spawn_spawn, and no point calling it again if it fails.
- */
-_hidden int libxl__spawn_detach(libxl__gc *gc,
- libxl__spawn_starting *for_spawn);
-
-/*
- * libxl__spawn_check - Check intermediate child process
- * gc: allocation pool
- * for_spawn: malloc'd pointer to libxl__spawn_starting (optional)
- *
- * Returns 0 on success, and < 0 on error.
- *
- * Logs errors but also returns them.
- * Caller must still call detach.
- */
-_hidden int libxl__spawn_check(libxl__gc *gc,
- libxl__spawn_starting *for_spawn);
/* low-level stuff, for synchronous subprocesses etc. */
@@ -984,15 +1081,6 @@ _hidden int libxl__domain_build(libxl__gc *gc,
/* for device model creation */
_hidden const char *libxl__domain_device_model(libxl__gc *gc,
const libxl_domain_build_info *info);
-_hidden int libxl__create_device_model(libxl__gc *gc,
- int domid,
- libxl_domain_config *guest_config,
- libxl__domain_build_state *state,
- libxl__spawner_starting **starting_r);
-_hidden int libxl__create_xenpv_qemu(libxl__gc *gc, uint32_t domid,
- libxl_domain_config *guest_config,
- libxl__domain_build_state *state,
- libxl__spawner_starting **starting_r);
_hidden int libxl__need_xenpv_qemu(libxl__gc *gc,
int nr_consoles, libxl__device_console *consoles,
int nr_vfbs, libxl_device_vfb *vfbs,
@@ -1000,10 +1088,6 @@ _hidden int libxl__need_xenpv_qemu(libxl__gc *gc,
/* Caller must either: pass starting_r==0, or on successful
* return pass *starting_r (which will be non-0) to
* libxl__confirm_device_model_startup or libxl__detach_device_model. */
-_hidden int libxl__confirm_device_model_startup(libxl__gc *gc,
- libxl__domain_build_state *state,
- libxl__spawner_starting *starting);
-_hidden int libxl__detach_device_model(libxl__gc *gc, libxl__spawner_starting *starting);
_hidden int libxl__wait_for_device_model(libxl__gc *gc,
uint32_t domid, char *state,
libxl__spawn_starting *spawning,
@@ -1566,6 +1650,32 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
_hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st);
+/*----- Domain creation -----*/
+
+typedef struct libxl__domain_create_state libxl__domain_create_state;
+
+typedef void libxl__domain_create_cb(libxl__egc *egc,
+ libxl__domain_create_state*,
+ int rc, uint32_t domid);
+
+struct libxl__domain_create_state {
+ /* filled in by user */
+ libxl__ao *ao;
+ libxl_domain_config *guest_config;
+ int restore_fd;
+ libxl_console_ready console_cb;
+ void *console_cb_priv;
+ libxl__domain_create_cb *callback;
+ /* private to domain_create */
+ int guest_domid;
+ libxl__domain_build_state build_state;
+ union {
+ libxl__dm_spawn_state dmss;
+ libxl__stubdom_spawn_state sdss;
+ };
+};
+
+
/*
* Convenience macros.
*/
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c9e9943..9e66536 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1668,8 +1668,8 @@ start:
if ( restore_file ) {
ret = libxl_domain_create_restore(ctx, &d_config,
- cb, &child_console_pid,
- &domid, restore_fd);
+ cb, &child_console_pid,
+ &domid, restore_fd, 0);
/*
* On subsequent reboot etc we should create the domain, not
* restore/migrate-receive it again.
@@ -1677,7 +1677,7 @@ start:
restore_file = NULL;
}else{
ret = libxl_domain_create_new(ctx, &d_config,
- cb, &child_console_pid, &domid);
+ cb, &child_console_pid, &domid, 0);
}
if ( ret )
goto error_out;
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 17/20] libxl: make libxl_create run bootloader via callback
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (15 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 16/20] libxl: ao: convert libxl__spawn_* Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 18/20] libxl: provide progress reporting for long-running operations Ian Jackson
` (2 subsequent siblings)
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Change initiate_domain_create to properly use libxl__bootloader_run
rather than improperly calling libxl_run_bootloader with NULL ao_how.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_create.c | 53 +++++++++++++++++++++++++++++++----------
tools/libxl/libxl_internal.h | 1 +
2 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 09a03a7..156c5c7 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -550,6 +550,9 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
static void domcreate_devmodel_started(libxl__egc *egc,
libxl__dm_spawn_state *dmss,
int rc);
+static void domcreate_bootloader_done(libxl__egc *egc,
+ libxl__bootloader_state *bl,
+ int rc);
/* Our own function to clean up and call the user's callback.
* The final call in the sequence. */
@@ -570,6 +573,7 @@ static void initiate_domain_create(libxl__egc *egc,
int restore_fd = dcs->restore_fd;
libxl_console_ready cb = dcs->console_cb;
void *priv = dcs->console_cb_priv;
+ memset(&dcs->build_state, 0, sizeof(dcs->build_state));
domid = 0;
@@ -602,18 +606,40 @@ static void initiate_domain_create(libxl__egc *egc,
libxl_device_disk *bootdisk =
d_config->num_disks > 0 ? &d_config->disks[0] : NULL;
- if (restore_fd < 0 && bootdisk) {
- ret = libxl_run_bootloader(ctx, &d_config->b_info, bootdisk, domid,
- 0 /* fixme-ao */);
- if (ret) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "failed to run bootloader: %d", ret);
- goto error_out;
- }
+ if (restore_fd < 0 && dcs->bl.disk) {
+ dcs->bl.ao = ao;
+ dcs->bl.disk = bootdisk;
+ dcs->bl.callback = domcreate_bootloader_done;
+ dcs->bl.info = &d_config->b_info,
+
+ libxl__bootloader_run(egc, &dcs->bl);
+ } else {
+ domcreate_bootloader_done(egc, &dcs->bl, 0);
}
+ return;
- memset(&dcs->build_state, 0, sizeof(dcs->build_state));
+error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
+static void domcreate_bootloader_done(libxl__egc *egc,
+ libxl__bootloader_state *bl,
+ int ret)
+{
+ libxl__domain_create_state *dcs = CONTAINER_OF(bl, *dcs, bl);
+ STATE_AO_GC(bl->ao);
+ int i;
+
+ /* convenience aliases */
+ uint32_t domid = dcs->guest_domid;
+ libxl_domain_config *d_config = dcs->guest_config;
+ int restore_fd = dcs->restore_fd;
libxl__domain_build_state *state = &dcs->build_state;
+ libxl_ctx *ctx = CTX;
+
+ if (ret) goto error_out;
+
dcs->dmss.spawn.ao = ao;
dcs->dmss.guest_config = dcs->guest_config;
dcs->dmss.build_state = &dcs->build_state;
@@ -764,12 +790,13 @@ static void domcreate_devmodel_started(libxl__egc *egc,
if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
libxl_defbool_val(d_config->b_info.u.pv.e820_host)) {
- int rc;
- rc = libxl__e820_alloc(gc, domid, d_config);
- if (rc)
+ ret = libxl__e820_alloc(gc, domid, d_config);
+ if (ret) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
"Failed while collecting E820 with: %d (errno:%d)\n",
- rc, errno);
+ ret, errno);
+ goto error_out;
+ }
}
if ( cb && (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM ||
(d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5bab4d6..56c3eec 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1669,6 +1669,7 @@ struct libxl__domain_create_state {
/* private to domain_create */
int guest_domid;
libxl__domain_build_state build_state;
+ libxl__bootloader_state bl;
union {
libxl__dm_spawn_state dmss;
libxl__stubdom_spawn_state sdss;
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 18/20] libxl: provide progress reporting for long-running operations
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (16 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 17/20] libxl: make libxl_create run bootloader via callback Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 19/20] libxl: remove malloc failure handling from NEW_EVENT Ian Jackson
2012-04-13 18:40 ` [PATCH 20/20] libxl: convert console callback to libxl_asyncprogress_how Ian Jackson
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
This will be used for reporting, during domain creation, that the
console is ready.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl.h | 45 +++++++++++++++
tools/libxl/libxl_event.c | 122 ++++++++++++++++++++++++++++++++----------
tools/libxl/libxl_internal.h | 28 ++++++++++
3 files changed, 167 insertions(+), 28 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6f59364..1bfe684 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -435,6 +435,51 @@ typedef struct {
} u;
} libxl_asyncop_how;
+/*
+ * Some more complex asynchronous operations can report intermediate
+ * progress. How this is to be reported is controlled, for each
+ * function, by a parameter
+ * libxl_asyncprogress_how *aop_FOO_how;
+ * for each kind of progress FOO supported by that function. Each
+ * such kind of progress is associated with an event type.
+ *
+ * The function description will document whether, when, and how
+ * many times, the intermediate progress will be reported, and
+ * what the corresponding event type(s) are.
+ *
+ * If aop_FOO_how==NULL, intermediate progress reports are discarded.
+ *
+ * If aop_FOO_how->callback==NULL, intermediate progress reports
+ * generate libxl events which can be obtained from libxl_event_wait
+ * or libxl_event_check.
+ *
+ * If aop_FOO_how->callback!=NULL, libxl will report intermediate
+ * progress by calling callback(ctx, &event, for_callback).
+ *
+ * The rules for these events are otherwise the same as those for
+ * ordinary events. The reentrancy and threading rules for the
+ * callback are the same as those for ao completion callbacks.
+ *
+ * Note that the callback, if provided, is responsible for freeing
+ * the event.
+ *
+ * If callbacks are requested, they will be made, and returned, before
+ * the long-running libxl operation is considered finished (so if the
+ * long-running libxl operation was invoked with ao_how==NULL then any
+ * callbacks will occur strictly before the long-running operation
+ * returns). However, the callbacks may occur on any thread.
+ *
+ * In general, otherwise, no promises are made about the relative
+ * order of callbacks in a multithreaded program. In particular
+ * different callbacks relating to the same long-running operation may
+ * be delivered out of order.
+ */
+
+typedef struct {
+ void (*callback)(libxl_ctx *ctx, libxl_event*, void *for_callback);
+ libxl_ev_user for_event; /* always used */
+ void *for_callback; /* passed to callback */
+} libxl_asyncprogress_how;
#define LIBXL_VERSION 0
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 1b7495a..f355e3d 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -899,12 +899,25 @@ static void egc_run_callbacks(libxl__egc *egc)
{
EGC_GC;
libxl_event *ev, *ev_tmp;
+ libxl__aop_occurred *aop, *aop_tmp;
LIBXL_TAILQ_FOREACH_SAFE(ev, &egc->occurred_for_callback, link, ev_tmp) {
LIBXL_TAILQ_REMOVE(&egc->occurred_for_callback, ev, link);
CTX->event_hooks->event_occurs(CTX->event_hooks_user, ev);
}
+ LIBXL_TAILQ_FOREACH_SAFE(aop, &egc->aops_for_callback, entry, aop_tmp) {
+ LIBXL_TAILQ_REMOVE(&egc->aops_for_callback, aop, entry);
+ aop->how->callback(CTX, aop->ev, aop->how->for_callback);
+
+ CTX_LOCK;
+ aop->ao->progress_reports_outstanding--;
+ libxl__ao_complete_check_progress_reports(egc, aop->ao);
+ CTX_UNLOCK;
+
+ free(aop);
+ }
+
libxl__ao *ao, *ao_tmp;
LIBXL_TAILQ_FOREACH_SAFE(ao, &egc->aos_for_callback,
entry_for_callback, ao_tmp) {
@@ -1285,6 +1298,7 @@ void libxl__ao_abort(libxl__ao *ao)
assert(ao->magic == LIBXL__AO_MAGIC);
assert(ao->in_initiator);
assert(!ao->complete);
+ assert(!ao->progress_reports_outstanding);
libxl__ao__destroy(CTX, ao);
}
@@ -1295,6 +1309,23 @@ void libxl__ao_complete(libxl__egc *egc, libxl__ao *ao, int rc)
ao->complete = 1;
ao->rc = rc;
+ libxl__ao_complete_check_progress_reports(egc, ao);
+}
+
+void libxl__ao_complete_check_progress_reports(libxl__egc *egc, libxl__ao *ao)
+{
+ /* We don't consider an ao complete if it has any outstanding
+ * callbacks. These callbacks might be outstanding on other
+ * threads, queued up in the other threads' egc's. Those threads
+ * will, after making the callback, take out the lock again,
+ * decrememt progress_reports_outstanding, and call us again.
+ */
+
+ assert(ao->progress_reports_outstanding >= 0);
+
+ if (!ao->complete || ao->progress_reports_outstanding)
+ return;
+
if (ao->poller) {
assert(ao->in_initiator);
if (!ao->constructing)
@@ -1316,34 +1347,6 @@ void libxl__ao_complete(libxl__egc *egc, libxl__ao *ao, int rc)
libxl__ao__destroy(libxl__gc_owner(&egc->gc), ao);
}
-libxl__ao *libxl__ao_create(libxl_ctx *ctx, uint32_t domid,
- const libxl_asyncop_how *how)
-{
- libxl__ao *ao;
-
- ao = calloc(1, sizeof(*ao));
- if (!ao) goto out;
-
- ao->magic = LIBXL__AO_MAGIC;
- ao->constructing = 1;
- ao->in_initiator = 1;
- ao->poller = 0;
- ao->domid = domid;
- LIBXL_INIT_GC(ao->gc, ctx);
-
- if (how) {
- ao->how = *how;
- } else {
- ao->poller = libxl__poller_get(ctx);
- if (!ao->poller) goto out;
- }
- return ao;
-
- out:
- if (ao) libxl__ao__destroy(ctx, ao);
- return NULL;
-}
-
int libxl__ao_inprogress(libxl__ao *ao)
{
AO_GC;
@@ -1401,6 +1404,69 @@ int libxl__ao_inprogress(libxl__ao *ao)
}
+/* progress reporting */
+
+/* The application indicates a desire to ignore events by passing NULL
+ * for how. But we want to copy *how. So we have this dummy function
+ * whose address is stored in callback if the app passed how==NULL. */
+static void dummy_asyncprogress_callback_ignore
+ (libxl_ctx *ctx, libxl_event *ev, void *for_callback) { }
+
+void libxl__ao_progress_gethow(libxl_asyncprogress_how *in_state,
+ const libxl_asyncprogress_how *from_app) {
+ if (from_app)
+ *in_state = *from_app;
+ else
+ in_state->callback = dummy_asyncprogress_callback_ignore;
+}
+
+void libxl__ao_progress_report(libxl__egc *egc, libxl__ao *ao,
+ const libxl_asyncprogress_how *how, libxl_event *ev)
+{
+ ev->for_user = how->for_event;
+ if (how->callback == dummy_asyncprogress_callback_ignore) {
+ /* ignore */
+ } else if (how->callback) {
+ libxl__aop_occurred *aop = libxl__zalloc(0, sizeof(*aop));
+ ao->progress_reports_outstanding++;
+ aop->ao = ao;
+ aop->ev = ev;
+ aop->how = how;
+ } else {
+ libxl__event_occurred(egc, ev);
+ }
+}
+
+
+libxl__ao *libxl__ao_create(libxl_ctx *ctx, uint32_t domid,
+ const libxl_asyncop_how *how)
+{
+ libxl__ao *ao;
+
+ ao = calloc(1, sizeof(*ao));
+ if (!ao) goto out;
+
+ ao->magic = LIBXL__AO_MAGIC;
+ ao->constructing = 1;
+ ao->in_initiator = 1;
+ ao->poller = 0;
+ ao->domid = domid;
+ LIBXL_INIT_GC(ao->gc, ctx);
+
+ if (how) {
+ ao->how = *how;
+ } else {
+ ao->poller = libxl__poller_get(ctx);
+ if (!ao->poller) goto out;
+ }
+ return ao;
+
+ out:
+ if (ao) libxl__ao__destroy(ctx, ao);
+ return NULL;
+}
+
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 56c3eec..4e97f5e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -118,6 +118,7 @@ _hidden void libxl__log(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
typedef struct libxl__gc libxl__gc;
typedef struct libxl__egc libxl__egc;
typedef struct libxl__ao libxl__ao;
+typedef struct libxl__aop_occurred libxl__aop_occurred;
void libxl__alloc_failed(libxl_ctx *, const char *func,
size_t nmemb, size_t size) __attribute__((noreturn));
@@ -372,6 +373,14 @@ struct libxl__egc {
struct libxl__gc gc;
struct libxl__event_list occurred_for_callback;
LIBXL_TAILQ_HEAD(, libxl__ao) aos_for_callback;
+ LIBXL_TAILQ_HEAD(, libxl__aop_occurred) aops_for_callback;
+};
+
+struct libxl__aop_occurred {
+ LIBXL_TAILQ_ENTRY(libxl__aop_occurred) entry;
+ libxl__ao *ao;
+ libxl_event *ev;
+ const libxl_asyncprogress_how *how;
};
#define LIBXL__AO_MAGIC 0xA0FACE00ul
@@ -380,6 +389,7 @@ struct libxl__egc {
struct libxl__ao {
uint32_t magic;
unsigned constructing:1, in_initiator:1, complete:1, notified:1;
+ int progress_reports_outstanding;
int rc;
libxl__gc gc;
libxl_asyncop_how how;
@@ -1369,6 +1379,7 @@ libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
LIBXL_INIT_GC((egc).gc,ctx); \
LIBXL_TAILQ_INIT(&(egc).occurred_for_callback); \
LIBXL_TAILQ_INIT(&(egc).aos_for_callback); \
+ LIBXL_TAILQ_INIT(&(egc).aops_for_callback); \
} while(0)
_hidden void libxl__egc_cleanup(libxl__egc *egc);
@@ -1415,6 +1426,9 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
* pointer to the internal event generation request routines
* libxl__evgen_FOO, so that at some point a CALLBACK will be
* made when the operation is complete.
+ * - if the operation provides progress reports, the aop_how(s)
+ * must be copied into the per-operation structure using
+ * libxl__ao_progress_gethow.
*
* - If initiation is successful, the initiating function needs
* to run libxl__ao_inprogress right before unlocking and
@@ -1424,6 +1438,10 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
* call libxl__ao_abort before unlocking and returning whatever
* error code is appropriate (AO_ABORT macro).
*
+ * - If the operation supports progress reports, it may generate
+ * suitable events with NEW_EVENT and report them with
+ * libxl__ao_progress_report (with the ctx locked).
+ *
* - Later, some callback function, whose callback has been requested
* directly or indirectly, should call libxl__ao_complete (with the
* ctx locked, as it will generally already be in any event callback
@@ -1479,8 +1497,18 @@ _hidden int libxl__ao_inprogress(libxl__ao *ao); /* temporarily unlocks */
_hidden void libxl__ao_abort(libxl__ao *ao);
_hidden void libxl__ao_complete(libxl__egc *egc, libxl__ao *ao, int rc);
+/* Can be called at any time. Use is essential for any aop user. */
+_hidden void libxl__ao_progress_gethow(libxl_asyncprogress_how *in_state,
+ const libxl_asyncprogress_how *from_app);
+
+/* Must be called with the ctx locked. Will fill in ev->for_user,
+ * so caller need not do that. */
+_hidden void libxl__ao_progress_report(libxl__egc *egc, libxl__ao *ao,
+ const libxl_asyncprogress_how *how, libxl_event *ev /* consumed */);
+
/* For use by ao machinery ONLY */
_hidden void libxl__ao__destroy(libxl_ctx*, libxl__ao *ao);
+_hidden void libxl__ao_complete_check_progress_reports(libxl__egc*, libxl__ao*);
/*
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 19/20] libxl: remove malloc failure handling from NEW_EVENT
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (17 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 18/20] libxl: provide progress reporting for long-running operations Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 20/20] libxl: convert console callback to libxl_asyncprogress_how Ian Jackson
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Change to use libxl__zalloc, where allocation failure is fatal.
Also remove a spurious semicolon from the helper macro.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl.c | 2 --
tools/libxl/libxl_event.c | 8 +-------
tools/libxl/libxl_internal.h | 4 ++--
3 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 54f3813..b7f665a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -779,7 +779,6 @@ static void domain_death_occurred(libxl__egc *egc,
*evg_upd = evg_next;
libxl_event *ev = NEW_EVENT(egc, DOMAIN_DEATH, evg->domid);
- if (!ev) return;
libxl__event_occurred(egc, ev);
@@ -866,7 +865,6 @@ static void domain_death_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *w,
if (!evg->shutdown_reported &&
(got->flags & XEN_DOMINF_shutdown)) {
libxl_event *ev = NEW_EVENT(egc, DOMAIN_SHUTDOWN, got->domain);
- if (!ev) goto out;
LIBXL__LOG(CTX, LIBXL__LOG_DEBUG, " shutdown reporting");
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index f355e3d..7600aa4 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -980,13 +980,7 @@ libxl_event *libxl__event_new(libxl__egc *egc,
{
libxl_event *ev;
- ev = malloc(sizeof(*ev));
- if (!ev) {
- LIBXL__EVENT_DISASTER(egc, "allocate new event", errno, type);
- return NULL;
- }
-
- memset(ev, 0, sizeof(*ev));
+ ev = libxl__zalloc(0,sizeof(*ev));
ev->type = type;
ev->domid = domid;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4e97f5e..977db2a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -641,10 +641,10 @@ _hidden libxl_event *libxl__event_new(libxl__egc*, libxl_event_type,
uint32_t domid);
/* Convenience function.
* Allocates a new libxl_event, fills in domid and type.
- * If allocation fails, calls _disaster, and returns NULL. */
+ * Cannot fail. */
#define NEW_EVENT(egc, type, domid) \
- libxl__event_new((egc), LIBXL_EVENT_TYPE_##type, (domid));
+ libxl__event_new((egc), LIBXL_EVENT_TYPE_##type, (domid))
/* Convenience macro. */
/*
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 20/20] libxl: convert console callback to libxl_asyncprogress_how
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
` (18 preceding siblings ...)
2012-04-13 18:40 ` [PATCH 19/20] libxl: remove malloc failure handling from NEW_EVENT Ian Jackson
@ 2012-04-13 18:40 ` Ian Jackson
19 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-13 18:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Stefano Stabellini
Remove the old console callback. (Its reentrancy properties were
troublesome: in principle, the event loop might be reentered during
the callback, and the libxl__domain_create_state swept out from under
the feet of the domain creation.)
As a side effect of the new code arrangements, the console callback
for non-bootloader-using PV guests now occurs near the end of domain
creation, in the same place as for HVM guests, rather than near the
start.
The new arrangements should in principle mean that by the time the
console is described as ready by the callback, the xenstore key is
indeed ready. So in the future the timeout might be removed from
the console client.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl.h | 17 ++++++-----
tools/libxl/libxl_bootloader.c | 3 ++
tools/libxl/libxl_create.c | 59 +++++++++++++++++++++++-----------------
tools/libxl/libxl_internal.h | 6 +++-
tools/libxl/libxl_types.idl | 2 +
tools/libxl/xl_cmdimpl.c | 25 ++++++++++-------
6 files changed, 67 insertions(+), 45 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 1bfe684..de8d1872 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -509,18 +509,19 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
/* domain related functions */
-typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
- /* fixme-ao Need to review this API. If we keep it, the reentrancy
- * properties need to be documented but they may turn out to be too
- * awkward */
int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
- libxl_console_ready cb, void *priv, uint32_t *domid,
- const libxl_asyncop_how *ao_how);
+ uint32_t *domid,
+ const libxl_asyncop_how *ao_how,
+ const libxl_asyncprogress_how *aop_console_how);
int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
- libxl_console_ready cb, void *priv,
uint32_t *domid, int restore_fd,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how,
+ const libxl_asyncprogress_how *aop_console_how);
+ /* A progress report will be made via ao_console_how, of type
+ * domain_create_console_available, when the domain's primary
+ * console is available and can be connected to.
+ */
void libxl_domain_config_dispose(libxl_domain_config *d_config);
int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info,
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index eaec46a..3f6f129 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -375,6 +375,9 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
goto out;
}
+ if (bl->console_available)
+ bl->console_available(egc, bl);
+
int bootloader_master = libxl__carefd_fd(bl->ptys[0].master);
int xenconsole_master = libxl__carefd_fd(bl->ptys[1].master);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 156c5c7..a457e09 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -550,10 +550,15 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
static void domcreate_devmodel_started(libxl__egc *egc,
libxl__dm_spawn_state *dmss,
int rc);
+static void domcreate_bootloader_console_available(libxl__egc *egc,
+ libxl__bootloader_state *bl);
static void domcreate_bootloader_done(libxl__egc *egc,
libxl__bootloader_state *bl,
int rc);
+static void domcreate_console_available(libxl__egc *egc,
+ libxl__domain_create_state *dcs);
+
/* Our own function to clean up and call the user's callback.
* The final call in the sequence. */
static void domcreate_complete(libxl__egc *egc,
@@ -571,8 +576,6 @@ static void initiate_domain_create(libxl__egc *egc,
/* convenience aliases */
libxl_domain_config *d_config = dcs->guest_config;
int restore_fd = dcs->restore_fd;
- libxl_console_ready cb = dcs->console_cb;
- void *priv = dcs->console_cb_priv;
memset(&dcs->build_state, 0, sizeof(dcs->build_state));
domid = 0;
@@ -590,11 +593,6 @@ static void initiate_domain_create(libxl__egc *egc,
dcs->guest_domid = domid;
dcs->dmss.guest_domid = 0; /* means we haven't spawned */
- if ( d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV && cb ) {
- ret = (*cb)(ctx, domid, priv);
- if (ret) goto error_out;
- }
-
ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
if (ret) goto error_out;
@@ -610,6 +608,7 @@ static void initiate_domain_create(libxl__egc *egc,
dcs->bl.ao = ao;
dcs->bl.disk = bootdisk;
dcs->bl.callback = domcreate_bootloader_done;
+ dcs->bl.console_available = domcreate_bootloader_console_available;
dcs->bl.info = &d_config->b_info,
libxl__bootloader_run(egc, &dcs->bl);
@@ -623,6 +622,21 @@ error_out:
domcreate_complete(egc, dcs, ret);
}
+static void domcreate_bootloader_console_available(libxl__egc *egc,
+ libxl__bootloader_state *bl)
+{
+ libxl__domain_create_state *dcs = CONTAINER_OF(bl, *dcs, bl);
+ STATE_AO_GC(bl->ao);
+ domcreate_console_available(egc, dcs);
+}
+
+static void domcreate_console_available(libxl__egc *egc,
+ libxl__domain_create_state *dcs) {
+ libxl__ao_progress_report(egc, dcs->ao, &dcs->aop_console_how,
+ NEW_EVENT(egc, DOMAIN_CREATE_CONSOLE_AVAILABLE,
+ dcs->guest_domid));
+}
+
static void domcreate_bootloader_done(libxl__egc *egc,
libxl__bootloader_state *bl,
int ret)
@@ -759,8 +773,6 @@ static void domcreate_devmodel_started(libxl__egc *egc,
/* convenience aliases */
libxl_domain_config *d_config = dcs->guest_config;
- libxl_console_ready cb = dcs->console_cb;
- void *priv = dcs->console_cb_priv;
if (ret) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
@@ -798,12 +810,7 @@ static void domcreate_devmodel_started(libxl__egc *egc,
goto error_out;
}
}
- if ( cb && (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM ||
- (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
- d_config->b_info.u.pv.bootloader ))) {
- ret = (*cb)(ctx, domid, priv);
- if (ret) goto error_out;
- }
+ domcreate_console_available(egc, dcs);
domcreate_complete(egc, dcs, 0);
return;
@@ -842,8 +849,9 @@ static void domain_create_cb(libxl__egc *egc,
int rc, uint32_t domid);
static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
- libxl_console_ready cb, void *priv, uint32_t *domid,
- int restore_fd, const libxl_asyncop_how *ao_how)
+ uint32_t *domid,
+ int restore_fd, const libxl_asyncop_how *ao_how,
+ const libxl_asyncprogress_how *aop_console_how)
{
AO_CREATE(ctx, 0, ao_how);
libxl__app_domain_create_state *cdcs;
@@ -852,9 +860,8 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
cdcs->dcs.ao = ao;
cdcs->dcs.guest_config = d_config;
cdcs->dcs.restore_fd = restore_fd;
- cdcs->dcs.console_cb = cb;
- cdcs->dcs.console_cb_priv = priv;
cdcs->dcs.callback = domain_create_cb;
+ libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how);
cdcs->domid_out = domid;
initiate_domain_create(egc, &cdcs->dcs);
@@ -876,19 +883,21 @@ static void domain_create_cb(libxl__egc *egc,
}
int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
- libxl_console_ready cb, void *priv,
uint32_t *domid,
- const libxl_asyncop_how *ao_how)
+ const libxl_asyncop_how *ao_how,
+ const libxl_asyncprogress_how *aop_console_how)
{
- return do_domain_create(ctx, d_config, cb, priv, domid, -1, ao_how);
+ return do_domain_create(ctx, d_config, domid, -1,
+ ao_how, aop_console_how);
}
int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
- libxl_console_ready cb, void *priv,
uint32_t *domid, int restore_fd,
- const libxl_asyncop_how *ao_how)
+ const libxl_asyncop_how *ao_how,
+ const libxl_asyncprogress_how *aop_console_how)
{
- return do_domain_create(ctx, d_config, cb, priv, domid, restore_fd, ao_how);
+ return do_domain_create(ctx, d_config, domid, restore_fd,
+ ao_how, aop_console_how);
}
/*
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 977db2a..b2eefd6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1651,11 +1651,14 @@ int libxl__openptys(libxl__openpty_state *op,
typedef struct libxl__bootloader_state libxl__bootloader_state;
typedef void libxl__run_bootloader_callback(libxl__egc*,
libxl__bootloader_state*, int rc);
+typedef void libxl__bootloader_console_callback(libxl__egc*,
+ libxl__bootloader_state*);
struct libxl__bootloader_state {
/* caller must fill these in, and they must all remain valid */
libxl__ao *ao;
libxl__run_bootloader_callback *callback;
+ libxl__bootloader_console_callback *console_available;
libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated */
libxl_device_disk *disk;
uint32_t domid;
@@ -1691,8 +1694,7 @@ struct libxl__domain_create_state {
libxl__ao *ao;
libxl_domain_config *guest_config;
int restore_fd;
- libxl_console_ready console_cb;
- void *console_cb_priv;
+ libxl_asyncprogress_how aop_console_how;
libxl__domain_create_cb *callback;
/* private to domain_create */
int guest_domid;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 5cf9708..f28d785 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -444,6 +444,7 @@ libxl_event_type = Enumeration("event_type", [
(2, "DOMAIN_DEATH"),
(3, "DISK_EJECT"),
(4, "OPERATION_COMPLETE"),
+ (5, "DOMAIN_CREATE_CONSOLE_AVAILABLE"),
])
libxl_ev_user = UInt(64)
@@ -469,4 +470,5 @@ libxl_event = Struct("event",[
("operation_complete", Struct(None, [
("rc", integer),
])),
+ ("domain_create_console_available", Struct(None, [])),
]))])
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 9e66536..ce52599 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1430,16 +1430,18 @@ static int freemem(libxl_domain_build_info *b_info)
return ERROR_NOMEM;
}
-static int autoconnect_console(libxl_ctx *ctx, uint32_t domid, void *priv)
+static void autoconnect_console(libxl_ctx *ctx, libxl_event *ev, void *priv)
{
pid_t *pid = priv;
+ libxl_event_free(ctx, ev);
+
*pid = fork();
if (*pid < 0) {
perror("unable to fork xenconsole");
- return ERROR_FAIL;
+ return;
} else if (*pid > 0)
- return 0;
+ return;
postfork();
@@ -1506,7 +1508,7 @@ static int create_domain(struct domain_create *dom_info)
int config_len = 0;
int restore_fd = -1;
int status = 0;
- libxl_console_ready cb;
+ const libxl_asyncprogress_how *autoconnect_console_how;
pid_t child_console_pid = -1;
struct save_file_header hdr;
@@ -1660,24 +1662,27 @@ start:
goto error_out;
}
+ libxl_asyncprogress_how autoconnect_console_how_buf;
if ( dom_info->console_autoconnect ) {
- cb = autoconnect_console;
+ autoconnect_console_how_buf.callback = autoconnect_console;
+ autoconnect_console_how_buf.for_callback = &child_console_pid;
+ autoconnect_console_how = &autoconnect_console_how_buf;
}else{
- cb = NULL;
+ autoconnect_console_how = 0;
}
if ( restore_file ) {
ret = libxl_domain_create_restore(ctx, &d_config,
- cb, &child_console_pid,
- &domid, restore_fd, 0);
+ &domid, restore_fd,
+ 0, autoconnect_console_how);
/*
* On subsequent reboot etc we should create the domain, not
* restore/migrate-receive it again.
*/
restore_file = NULL;
}else{
- ret = libxl_domain_create_new(ctx, &d_config,
- cb, &child_console_pid, &domid, 0);
+ ret = libxl_domain_create_new(ctx, &d_config, &domid,
+ 0, autoconnect_console_how);
}
if ( ret )
goto error_out;
--
1.7.2.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 01/20] libxl: handle POLLERR, POLLHUP, POLLNVAL properly
2012-04-13 18:39 ` [PATCH 01/20] libxl: handle POLLERR, POLLHUP, POLLNVAL properly Ian Jackson
@ 2012-04-16 7:53 ` Ian Campbell
0 siblings, 0 replies; 36+ messages in thread
From: Ian Campbell @ 2012-04-16 7:53 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
On Fri, 2012-04-13 at 19:39 +0100, Ian Jackson wrote:
> Pass POLLERR and POLLHUP to fd callbacks, as is necessary.
> Crash on POLLNVAL since that means our fds are messed up.
>
> Document the behaviour (including the fact that poll sometimes sets
> POLLHUP or POLLERR even if only POLLIN was requested.
>
> Fix the one current fd callback to do something with POLLERR|POLLHUP.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_event.c | 7 ++++++-
> tools/libxl/libxl_internal.h | 5 +++++
> 2 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index 638b9ab..5e1a207 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -335,6 +335,9 @@ static void watchfd_callback(libxl__egc *egc, libxl__ev_fd *ev,
> {
> EGC_GC;
>
> + if (revents & (POLLERR|POLLHUP))
> + LIBXL__EVENT_DISASTER(egc, "unexpected poll event on watch fd", 0, 0);
> +
> for (;;) {
> char **event = xs_check_watch(CTX->xsh);
> if (!event) {
> @@ -739,7 +742,9 @@ static int afterpoll_check_fd(libxl__poller *poller,
> /* again, stale slot entry */
> return 0;
>
> - int revents = fds[slot].revents & events;
> + assert(!(fds[slot].revents & POLLNVAL));
> +
> + int revents = fds[slot].revents & (events | POLLERR | POLLHUP);
> /* we mask in case requested events have changed */
>
> return revents;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index a4b933b..af631fb 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -127,6 +127,11 @@ void libxl__alloc_failed(libxl_ctx *, const char *func,
> typedef struct libxl__ev_fd libxl__ev_fd;
> typedef void libxl__ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
> int fd, short events, short revents);
> + /* Note that revents may contain POLLERR or POLLHUP regardless of
> + * events; otherwise revents contains only bits in events. Contrary
> + * to the documentation for poll(2), POLLERR and POLLHUP can occur
> + * even if only POLLIN was set in events. (POLLNVAL is a fatal
> + * error and will cause libxl event machinery to fail an assertion.) */
> struct libxl__ev_fd {
> /* caller should include this in their own struct */
> /* read-only for caller, who may read only when registered: */
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 02/20] libxl: support multiple libxl__ev_fds for the same fd
2012-04-13 18:39 ` [PATCH 02/20] libxl: support multiple libxl__ev_fds for the same fd Ian Jackson
@ 2012-04-16 8:04 ` Ian Campbell
2012-04-16 10:26 ` Ian Jackson
0 siblings, 1 reply; 36+ messages in thread
From: Ian Campbell @ 2012-04-16 8:04 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
On Fri, 2012-04-13 at 19:39 +0100, Ian Jackson wrote:
> We need a slightly more sophisticated data structure to allow this,
> where we record the slot not just for each fd but also for each
> (fd,eventbit) where eventbit is POLLIN, POLLPRI, POLLOUT.
Just to be sure I'm following: By multiple you mean you can have one
libxl__ev_fds listening for e.g. POLLIN and another for POLLOUT but you
specifically exclude the case where two libxl__ev_fds both want to
listen for POLLIN? Similarly one listening for POLLIN|POLLPRI and the
other for POLLOUT|POLLPRI (overlapping) is forbidden.
> Document the new relaxed restriction.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> tools/libxl/libxl_event.c | 62 +++++++++++++++++++++++------------------
> tools/libxl/libxl_internal.h | 10 +++++--
> 2 files changed, 42 insertions(+), 30 deletions(-)
>
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index 5e1a207..672e3fe 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -635,10 +635,11 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
>
>
> /*
> - * In order to be able to efficiently find the libxl__ev_fd
> - * for a struct poll during _afterpoll, we maintain a shadow
> - * data structure in CTX->fd_beforepolled: each slot in
> - * the fds array corresponds to a slot in fd_beforepolled.
> + * In order to be able to efficiently find the libxl__ev_fd for a
> + * struct poll during _afterpoll, we maintain a shadow data
> + * structure in CTX->fd_rindices: each fd corresponds to a slot in
> + * fd_rindices, and each elemebnt in the rindices is three indices
element
> + * into the fd array (for POLLIN, POLLPRI and POLLOUT).
> */
>
> if (*nfds_io) {
> @@ -659,14 +660,16 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
> });
>
> /* make sure our array is as big as *nfds_io */
> - if (poller->fd_rindex_allocd < maxfd) {
> - assert(maxfd < INT_MAX / sizeof(int) / 2);
> - int *newarray = realloc(poller->fd_rindex, sizeof(int) * maxfd);
> - if (!newarray) { rc = ERROR_NOMEM; goto out; }
> - memset(newarray + poller->fd_rindex_allocd, 0,
> - sizeof(int) * (maxfd - poller->fd_rindex_allocd));
> - poller->fd_rindex = newarray;
> - poller->fd_rindex_allocd = maxfd;
> + if (poller->fd_rindices_allocd < maxfd) {
> + assert(ARRAY_SIZE_OK(poller->fd_rindices, maxfd));
> + poller->fd_rindices =
> + libxl__realloc(0, poller->fd_rindices,
> + maxfd * sizeof(*poller->fd_rindices));
> + memset(poller->fd_rindices + poller->fd_rindices_allocd,
> + 0,
> + (maxfd - poller->fd_rindices_allocd)
> + * sizeof(*poller->fd_rindices));
> + poller->fd_rindices_allocd = maxfd;
> }
> }
>
> @@ -677,8 +680,10 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
> fds[used].fd = req_fd;
> fds[used].events = req_events;
> fds[used].revents = 0;
> - assert(req_fd < poller->fd_rindex_allocd);
> - poller->fd_rindex[req_fd] = used;
> + assert(req_fd < poller->fd_rindices_allocd);
> + if (req_events & POLLIN) poller->fd_rindices[req_fd][0] = used;
> + if (req_events & POLLPRI) poller->fd_rindices[req_fd][1] = used;
> + if (req_events & POLLOUT) poller->fd_rindices[req_fd][2] = used;
Would it be possible to have a little struct here instead of the [3]? So
you'd get
poller->fd_rindeices[req_fd].{in,pri,out}
?
Ah, I see this would make afterpoll_check_fd more complex which I think
would outweigh any gain here.
So, other than the typoe:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/20] autoconf: New test for openpty et al.
2012-04-13 18:39 ` [PATCH 04/20] autoconf: New test for openpty et al Ian Jackson
@ 2012-04-16 9:09 ` Ian Campbell
2012-04-16 10:32 ` Ian Jackson
2012-04-16 10:09 ` Roger Pau Monne
1 sibling, 1 reply; 36+ messages in thread
From: Ian Campbell @ 2012-04-16 9:09 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
On Fri, 2012-04-13 at 19:39 +0100, Ian Jackson wrote:
> We may need to #include <libutil.h>, and/or link with -lutil, to use
> openpty, login_tty, and the like. Provide INCLUDE_LIBUTIL_H
> (preprocessor constant, not always defined) and PTYFUNCS_LIBS
> (makefile variable).
>
> We link libxl against PTYFUNCS_LIBS (which comes from autoconf) rather
> than UTIL_LIBS, and #include <libutil.h> where appropriate.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> config/Tools.mk.in | 2 +
> tools/configure | 51 ++++++++++++++++++++++++++++++++++++++++
> tools/configure.ac | 2 +
> tools/libxl/Makefile | 2 +-
> tools/libxl/libxl_bootloader.c | 4 +++
> tools/m4/ptyfuncs.m4 | 24 ++++++++++++++++++
> 6 files changed, 84 insertions(+), 1 deletions(-)
> create mode 100644 tools/m4/ptyfuncs.m4
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index e5ea867..5ba144f 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid
> endif
>
> LIBXL_LIBS =
> -LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(UTIL_LIBS) $(LIBUUID_LIBS)
> +LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
Did you intend to remove the definition of UTIL_LIBS? I don't see a hunk
which does that.
> diff --git a/tools/m4/ptyfuncs.m4 b/tools/m4/ptyfuncs.m4
> new file mode 100644
> index 0000000..2846a6d
> --- /dev/null
> +++ b/tools/m4/ptyfuncs.m4
> @@ -0,0 +1,24 @@
> +AC_DEFUN([AX_CHECK_PTYFUNCS], [
> + AC_CHECK_HEADER([libutil.h],[
> + AC_DEFINE([INCLUDE_LIBUTIL_H],[<libutil.h>],[libutil header file name])
> + ])
> + AC_CACHE_CHECK([for openpty et al], [ax_cv_ptyfuncs_libs], [
> + ax_cv_ptyfuncs_libs=-lutil
> + AX_SAVEVAR_SAVE(LIBS)
> + LIBS="$LIBS $ax_cv_ptyfuncs_libs"
> + AC_LINK_IFELSE([
> +#ifdef INCLUDE_LIBUTIL_H
> +#include INCLUDE_LIBUTIL_H
> +#endif
> +int main(void) {
> + openpty(0,0,0,0,0);
> + login_tty(0);
> +}
> +])
> + AX_SAVEVAR_RESTORE(LIBS)],
> + [],[
> + AC_MSG_FAILURE([Unable to find -lutil for openpty and login_tty])
> + ])
> + PTYFUNCS_LIBS="$ax_cv_ptyfuncs_libs"
> + AC_SUBST(PTYFUNCS_LIBS)
> +])
Does this fail if -lutil isn't available? My (perhaps mistaken) reading
of the commit message was that -lutil was only needed on some platforms?
On the other hand the AC_MSG_FAILURE doesn't see to have appeared in the
configure output -- so maybe I just understand what all these macros are
actually doing.
Ian.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 05/20] libxl: provide libxl__remove_file et al.
2012-04-13 18:39 ` [PATCH 05/20] libxl: provide libxl__remove_file " Ian Jackson
@ 2012-04-16 9:24 ` Ian Campbell
2012-04-16 10:33 ` Ian Jackson
0 siblings, 1 reply; 36+ messages in thread
From: Ian Campbell @ 2012-04-16 9:24 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
> +int libxl__remove_directory(libxl__gc *gc, const char *dirpath)
> +{
> + int rc = 0;
> + DIR *d = 0;
> +
> + d = opendir(dirpath);
> + if (!d) {
> + if (errno == ENOENT)
> + goto out;
> +
> + LOGE(ERROR, "failed to opendir %s for removal", dirpath);
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + size_t need = offsetof(struct dirent, d_name) +
> + pathconf(dirpath, _PC_NAME_MAX) + 1;
This was an interesting pattern, but I see that readdir(3) recommends
portable programs do things this way...
> + struct dirent *de_buf = libxl__zalloc(gc, need);
> + struct dirent *de;
> +
> + for (;;) {
> + int r = readdir_r(d, de_buf, &de);
> + if (r) {
> + LOGE(ERROR, "failed to readdir %s for removal", dirpath);
> + rc = ERROR_FAIL;
> + break;
> + }
> + if (!de)
> + break;
> +
> + if (!strcmp(de->d_name, ".") ||
> + !strcmp(de->d_name, ".."))
> + continue;
> +
> + const char *subpath = GCSPRINTF("%s/%s", dirpath, de->d_name);
> + if (libxl__remove_file_or_directory(gc, subpath))
> + rc = ERROR_FAIL;
Deliberately doesn't "goto out" so as to remove as much as possible?
If so then:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> + }
> +
> + for (;;) {
> + int r = rmdir(dirpath);
> + if (!r) break;
> + if (errno == ENOENT) goto out;
> + if (errno == EINTR) continue;
> + LOGE(ERROR, "failed to remove emptied directory %s", dirpath);
> + rc = ERROR_FAIL;
> + }
> +
> + out:
> + if (d) closedir(d);
> +
> + return rc;
> +}
>
> pid_t libxl_fork(libxl_ctx *ctx)
> {
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/20] autoconf: New test for openpty et al.
2012-04-13 18:39 ` [PATCH 04/20] autoconf: New test for openpty et al Ian Jackson
2012-04-16 9:09 ` Ian Campbell
@ 2012-04-16 10:09 ` Roger Pau Monne
2012-04-16 10:35 ` Ian Jackson
1 sibling, 1 reply; 36+ messages in thread
From: Roger Pau Monne @ 2012-04-16 10:09 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
El 13/04/2012, a las 19:39, Ian Jackson escribió:
> We may need to #include <libutil.h>, and/or link with -lutil, to use
> openpty, login_tty, and the like. Provide INCLUDE_LIBUTIL_H
> (preprocessor constant, not always defined) and PTYFUNCS_LIBS
> (makefile variable).
>
> We link libxl against PTYFUNCS_LIBS (which comes from autoconf) rather
> than UTIL_LIBS, and #include <libutil.h> where appropriate.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> config/Tools.mk.in | 2 +
> tools/configure | 51 ++++++++++++++++++++++++++++++++++++++++
> tools/configure.ac | 2 +
> tools/libxl/Makefile | 2 +-
> tools/libxl/libxl_bootloader.c | 4 +++
> tools/m4/ptyfuncs.m4 | 24 ++++++++++++++++++
> 6 files changed, 84 insertions(+), 1 deletions(-)
> create mode 100644 tools/m4/ptyfuncs.m4
>
> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index 2774062..b50944a 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -16,6 +16,10 @@
>
> #include <termios.h>
>
> +#ifdef INCLUDE_LIBUTIL_H
> +#include INCLUDE_LIBUTIL_H
> +#endif
> +
Maybe I'm missing something, because this autoconf macros are hard to read, but shouldn't you add something like
#undef INCLUDE_LIBUTIL_H
to tools/config.h.in, so the generated config.h contains this define?
> #include "libxl_internal.h"
>
> #define XENCONSOLED_BUF_SIZE 16
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 02/20] libxl: support multiple libxl__ev_fds for the same fd
2012-04-16 8:04 ` Ian Campbell
@ 2012-04-16 10:26 ` Ian Jackson
2012-04-16 11:17 ` Ian Campbell
0 siblings, 1 reply; 36+ messages in thread
From: Ian Jackson @ 2012-04-16 10:26 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
Ian Campbell writes ("Re: [Xen-devel] [PATCH 02/20] libxl: support multiple libxl__ev_fds for the same fd"):
> On Fri, 2012-04-13 at 19:39 +0100, Ian Jackson wrote:
> > We need a slightly more sophisticated data structure to allow this,
> > where we record the slot not just for each fd but also for each
> > (fd,eventbit) where eventbit is POLLIN, POLLPRI, POLLOUT.
>
> Just to be sure I'm following: By multiple you mean you can have one
> libxl__ev_fds listening for e.g. POLLIN and another for POLLOUT but you
> specifically exclude the case where two libxl__ev_fds both want to
> listen for POLLIN? Similarly one listening for POLLIN|POLLPRI and the
> other for POLLOUT|POLLPRI (overlapping) is forbidden.
Yes, exactly. As I say in the new doc comment:
+ *
+ * It is not permitted to listen for the same or overlapping events
+ * on the same fd using multiple different libxl__ev_fd's.
This restriction is actually stronger than that required by the code.
The code merely requires that for every distinct libxl__ev_fd
listening on the same fd, it has at least one of the three flags all
to itself. So your second scenario would actually work.
Would it be worth documenting this precise restriction ?
> > + * fd_rindices, and each elemebnt in the rindices is three indices
>
> element
Fixed.
> So, other than the typoe:
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks.
Ian.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/20] autoconf: New test for openpty et al.
2012-04-16 9:09 ` Ian Campbell
@ 2012-04-16 10:32 ` Ian Jackson
2012-04-16 11:19 ` Ian Campbell
0 siblings, 1 reply; 36+ messages in thread
From: Ian Jackson @ 2012-04-16 10:32 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
Ian Campbell writes ("Re: [Xen-devel] [PATCH 04/20] autoconf: New test for openpty et al."):
> On Fri, 2012-04-13 at 19:39 +0100, Ian Jackson wrote:
> > LIBXL_LIBS =
> > -LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(UTIL_LIBS) $(LIBUUID_LIBS)
> > +LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
>
> Did you intend to remove the definition of UTIL_LIBS? I don't see a hunk
> which does that.
It's also used by xenconsoled.
> > +AC_DEFUN([AX_CHECK_PTYFUNCS], [
> > + AC_CHECK_HEADER([libutil.h],[
> > + AC_DEFINE([INCLUDE_LIBUTIL_H],[<libutil.h>],[libutil header file name])
> > + ])
> > + AC_CACHE_CHECK([for openpty et al], [ax_cv_ptyfuncs_libs], [
> > + ax_cv_ptyfuncs_libs=-lutil
> > + AX_SAVEVAR_SAVE(LIBS)
> > + LIBS="$LIBS $ax_cv_ptyfuncs_libs"
> > + AC_LINK_IFELSE([
> > +#ifdef INCLUDE_LIBUTIL_H
> > +#include INCLUDE_LIBUTIL_H
> > +#endif
> > +int main(void) {
> > + openpty(0,0,0,0,0);
> > + login_tty(0);
> > +}
> > +])
> > + AX_SAVEVAR_RESTORE(LIBS)],
> > + [],[
> > + AC_MSG_FAILURE([Unable to find -lutil for openpty and login_tty])
> > + ])
> > + PTYFUNCS_LIBS="$ax_cv_ptyfuncs_libs"
> > + AC_SUBST(PTYFUNCS_LIBS)
> > +])
>
> Does this fail if -lutil isn't available? My (perhaps mistaken) reading
> of the commit message was that -lutil was only needed on some platforms?
Yes, it will do. Currently -lutil is used everywhere except Solaris
but surely Solaris support has rotted by now. I could change the
patch to try both with and without, I guess.
> On the other hand the AC_MSG_FAILURE doesn't see to have appeared in the
> configure output -- so maybe I just understand what all these macros are
> actually doing.
Linux has -lutil.
Ian.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 05/20] libxl: provide libxl__remove_file et al.
2012-04-16 9:24 ` Ian Campbell
@ 2012-04-16 10:33 ` Ian Jackson
0 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-16 10:33 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
Ian Campbell writes ("Re: [Xen-devel] [PATCH 05/20] libxl: provide libxl__remove_file et al."):
> > + size_t need = offsetof(struct dirent, d_name) +
> > + pathconf(dirpath, _PC_NAME_MAX) + 1;
>
> This was an interesting pattern, but I see that readdir(3) recommends
> portable programs do things this way...
Bizarre, eh ?
> > + const char *subpath = GCSPRINTF("%s/%s", dirpath, de->d_name);
> > + if (libxl__remove_file_or_directory(gc, subpath))
> > + rc = ERROR_FAIL;
>
> Deliberately doesn't "goto out" so as to remove as much as possible?
Yes.
> If so then:
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks,
Ian.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/20] autoconf: New test for openpty et al.
2012-04-16 10:09 ` Roger Pau Monne
@ 2012-04-16 10:35 ` Ian Jackson
2012-04-16 10:40 ` Roger Pau Monne
0 siblings, 1 reply; 36+ messages in thread
From: Ian Jackson @ 2012-04-16 10:35 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
Roger Pau Monne writes ("Re: [Xen-devel] [PATCH 04/20] autoconf: New test for openpty et al."):
> > +#ifdef INCLUDE_LIBUTIL_H
> > +#include INCLUDE_LIBUTIL_H
> > +#endif
> > +
>
> Maybe I'm missing something, because this autoconf macros are hard to read, but shouldn't you add something like
>
> #undef INCLUDE_LIBUTIL_H
>
> to tools/config.h.in, so the generated config.h contains this define?
Yes. This is normally done by autoheader, which is normally done by
autogen.sh. But I see that we aren't running autoheader. I will see
about fixing that.
Ian.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/20] autoconf: New test for openpty et al.
2012-04-16 10:35 ` Ian Jackson
@ 2012-04-16 10:40 ` Roger Pau Monne
2012-04-16 10:56 ` Ian Jackson
0 siblings, 1 reply; 36+ messages in thread
From: Roger Pau Monne @ 2012-04-16 10:40 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
El 16/04/2012, a las 11:35, Ian Jackson escribió:
> Roger Pau Monne writes ("Re: [Xen-devel] [PATCH 04/20] autoconf: New test for openpty et al."):
>>> +#ifdef INCLUDE_LIBUTIL_H
>>> +#include INCLUDE_LIBUTIL_H
>>> +#endif
>>> +
>>
>> Maybe I'm missing something, because this autoconf macros are hard to read, but shouldn't you add something like
>>
>> #undef INCLUDE_LIBUTIL_H
>>
>> to tools/config.h.in, so the generated config.h contains this define?
>
> Yes. This is normally done by autoheader, which is normally done by
> autogen.sh. But I see that we aren't running autoheader. I will see
> about fixing that.
>
Autoheader was adding a lot of stuff which we weren't using, so I preferred to keep config.h.in (and config.h) with just the defines we really need.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/20] autoconf: New test for openpty et al.
2012-04-16 10:40 ` Roger Pau Monne
@ 2012-04-16 10:56 ` Ian Jackson
0 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-16 10:56 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
Roger Pau Monne writes ("Re: [Xen-devel] [PATCH 04/20] autoconf: New test for openpty et al."):
> El 16/04/2012, a las 11:35, Ian Jackson escribió:
> > Yes. This is normally done by autoheader, which is normally done by
> > autogen.sh. But I see that we aren't running autoheader. I will see
> > about fixing that.
>
> Autoheader was adding a lot of stuff which we weren't using, so I preferred to keep config.h.in (and config.h) with just the defines we really need.
I just ran it and everything it generates seems to be produced by
something in configure.ac, as I expected.
Surely the problem then is that configure.ac has too much useless
stuff in it ? For example why are we checking for things like alarm
and atexit and everything from <string.h>, which are going to exist
everywhere ?
Ian.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 02/20] libxl: support multiple libxl__ev_fds for the same fd
2012-04-16 10:26 ` Ian Jackson
@ 2012-04-16 11:17 ` Ian Campbell
0 siblings, 0 replies; 36+ messages in thread
From: Ian Campbell @ 2012-04-16 11:17 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
On Mon, 2012-04-16 at 11:26 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 02/20] libxl: support multiple libxl__ev_fds for the same fd"):
> > On Fri, 2012-04-13 at 19:39 +0100, Ian Jackson wrote:
> > > We need a slightly more sophisticated data structure to allow this,
> > > where we record the slot not just for each fd but also for each
> > > (fd,eventbit) where eventbit is POLLIN, POLLPRI, POLLOUT.
> >
> > Just to be sure I'm following: By multiple you mean you can have one
> > libxl__ev_fds listening for e.g. POLLIN and another for POLLOUT but you
> > specifically exclude the case where two libxl__ev_fds both want to
> > listen for POLLIN? Similarly one listening for POLLIN|POLLPRI and the
> > other for POLLOUT|POLLPRI (overlapping) is forbidden.
>
> Yes, exactly. As I say in the new doc comment:
> + *
> + * It is not permitted to listen for the same or overlapping events
> + * on the same fd using multiple different libxl__ev_fd's.
>
> This restriction is actually stronger than that required by the code.
> The code merely requires that for every distinct libxl__ev_fd
> listening on the same fd, it has at least one of the three flags all
> to itself. So your second scenario would actually work.
>
> Would it be worth documenting this precise restriction ?
I guess it would just confuse the language but only really add anything
for what is a pretty small corner case?
Since it's a libxl internal thing I'm not too concerned about people
coming to rely on the actual broader behaviour.
>
> > > + * fd_rindices, and each elemebnt in the rindices is three indices
> >
> > element
>
> Fixed.
>
> > So, other than the typoe:
> >
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Thanks.
>
> Ian.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/20] autoconf: New test for openpty et al.
2012-04-16 10:32 ` Ian Jackson
@ 2012-04-16 11:19 ` Ian Campbell
2012-04-16 11:50 ` Ian Jackson
0 siblings, 1 reply; 36+ messages in thread
From: Ian Campbell @ 2012-04-16 11:19 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
On Mon, 2012-04-16 at 11:32 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 04/20] autoconf: New test for openpty et al."):
> > On Fri, 2012-04-13 at 19:39 +0100, Ian Jackson wrote:
> > > LIBXL_LIBS =
> > > -LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(UTIL_LIBS) $(LIBUUID_LIBS)
> > > +LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
> >
> > Did you intend to remove the definition of UTIL_LIBS? I don't see a hunk
> > which does that.
>
> It's also used by xenconsoled.
>
> > > +AC_DEFUN([AX_CHECK_PTYFUNCS], [
> > > + AC_CHECK_HEADER([libutil.h],[
> > > + AC_DEFINE([INCLUDE_LIBUTIL_H],[<libutil.h>],[libutil header file name])
> > > + ])
> > > + AC_CACHE_CHECK([for openpty et al], [ax_cv_ptyfuncs_libs], [
> > > + ax_cv_ptyfuncs_libs=-lutil
> > > + AX_SAVEVAR_SAVE(LIBS)
> > > + LIBS="$LIBS $ax_cv_ptyfuncs_libs"
> > > + AC_LINK_IFELSE([
> > > +#ifdef INCLUDE_LIBUTIL_H
> > > +#include INCLUDE_LIBUTIL_H
> > > +#endif
> > > +int main(void) {
> > > + openpty(0,0,0,0,0);
> > > + login_tty(0);
> > > +}
> > > +])
> > > + AX_SAVEVAR_RESTORE(LIBS)],
> > > + [],[
> > > + AC_MSG_FAILURE([Unable to find -lutil for openpty and login_tty])
> > > + ])
> > > + PTYFUNCS_LIBS="$ax_cv_ptyfuncs_libs"
> > > + AC_SUBST(PTYFUNCS_LIBS)
> > > +])
> >
> > Does this fail if -lutil isn't available? My (perhaps mistaken) reading
> > of the commit message was that -lutil was only needed on some platforms?
>
> Yes, it will do. Currently -lutil is used everywhere except Solaris
> but surely Solaris support has rotted by now. I could change the
> patch to try both with and without, I guess.
OK, I think we can just leave it until someone ports Xen to a platform
which has this behaviour (i.e. it can be tested)
> > On the other hand the AC_MSG_FAILURE doesn't see to have appeared in the
> > configure output -- so maybe I just understand what all these macros are
> > actually doing.
>
> Linux has -lutil.
I meant that the message doesn't seem to appear in the generate
configure script itself.
Ian.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/20] autoconf: New test for openpty et al.
2012-04-16 11:19 ` Ian Campbell
@ 2012-04-16 11:50 ` Ian Jackson
0 siblings, 0 replies; 36+ messages in thread
From: Ian Jackson @ 2012-04-16 11:50 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
Ian Campbell writes ("Re: [Xen-devel] [PATCH 04/20] autoconf: New test for openpty et al."):
> On Mon, 2012-04-16 at 11:32 +0100, Ian Jackson wrote:
> > > Does this fail if -lutil isn't available? My (perhaps mistaken) reading
> > > of the commit message was that -lutil was only needed on some platforms?
> >
> > Yes, it will do. Currently -lutil is used everywhere except Solaris
> > but surely Solaris support has rotted by now. I could change the
> > patch to try both with and without, I guess.
>
> OK, I think we can just leave it until someone ports Xen to a platform
> which has this behaviour (i.e. it can be tested)
I've updated it anyway, as it wasn't too hard.
> > > On the other hand the AC_MSG_FAILURE doesn't see to have appeared in the
> > > configure output -- so maybe I just understand what all these macros are
> > > actually doing.
> >
> > Linux has -lutil.
>
> I meant that the message doesn't seem to appear in the generate
> configure script itself.
While editing this patch I found that I had some of the bracketing
wrong. That was probably it. It'll be fixed in v2.
Ian.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 13/20] libxl: Allow AO_GC and EGC_GC even if not used
2012-04-13 18:40 ` [PATCH 13/20] libxl: Allow AO_GC and EGC_GC even if not used Ian Jackson
@ 2012-04-16 14:43 ` Ian Campbell
0 siblings, 0 replies; 36+ messages in thread
From: Ian Campbell @ 2012-04-16 14:43 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
On Fri, 2012-04-13 at 19:40 +0100, Ian Jackson wrote:
> Mark the gc produced by AO_GC and EGC_GC with the gcc feature
> __attribute__((unused)). This allows the use of EGC_INIT and
> STATE_AO_GC by functions which do actually use the gc.
>
> This is convenient because those functions might want to use the ao or
> egc, rather than the gc; and also because it means that functions
> which morally ought to be fishing any gc they use out of an egc or
> state structure can be written do so regardless of whether the gc is
> actually used right then.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Needed this for one of my patches (just sent, with a reference to this
patch). So:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_internal.h | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 4cfb8d5..74dc2c5 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1280,7 +1280,7 @@ libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
> /* useful for all functions which take an egc: */
>
> #define EGC_GC \
> - libxl__gc *const gc = &egc->gc
> + libxl__gc *const gc __attribute__((unused)) = &egc->gc
>
> /* egc initialisation and destruction: */
>
> @@ -1383,7 +1383,7 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
> })
>
> #define AO_GC \
> - libxl__gc *const gc = &ao->gc
> + libxl__gc *const gc __attribute__((unused)) = &ao->gc
>
> #define STATE_AO_GC(op_ao) \
> libxl__ao *const ao = (op_ao); \
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2012-04-16 14:43 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-13 18:39 [PATCH v6 00/20] libxl: subprocess handling Ian Jackson
2012-04-13 18:39 ` [PATCH 01/20] libxl: handle POLLERR, POLLHUP, POLLNVAL properly Ian Jackson
2012-04-16 7:53 ` Ian Campbell
2012-04-13 18:39 ` [PATCH 02/20] libxl: support multiple libxl__ev_fds for the same fd Ian Jackson
2012-04-16 8:04 ` Ian Campbell
2012-04-16 10:26 ` Ian Jackson
2012-04-16 11:17 ` Ian Campbell
2012-04-13 18:39 ` [PATCH 03/20] libxl: event API: new facilities for waiting for subprocesses Ian Jackson
2012-04-13 18:39 ` [PATCH 04/20] autoconf: New test for openpty et al Ian Jackson
2012-04-16 9:09 ` Ian Campbell
2012-04-16 10:32 ` Ian Jackson
2012-04-16 11:19 ` Ian Campbell
2012-04-16 11:50 ` Ian Jackson
2012-04-16 10:09 ` Roger Pau Monne
2012-04-16 10:35 ` Ian Jackson
2012-04-16 10:40 ` Roger Pau Monne
2012-04-16 10:56 ` Ian Jackson
2012-04-13 18:39 ` [PATCH 05/20] libxl: provide libxl__remove_file " Ian Jackson
2012-04-16 9:24 ` Ian Campbell
2012-04-16 10:33 ` Ian Jackson
2012-04-13 18:40 ` [PATCH 06/20] libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds Ian Jackson
2012-04-13 18:40 ` [PATCH 07/20] libxl: Clean up setdefault in do_domain_create Ian Jackson
2012-04-13 18:40 ` [PATCH 08/20] libxl: provide libxl__datacopier_* Ian Jackson
2012-04-13 18:40 ` [PATCH 09/20] libxl: provide libxl__openpty_* Ian Jackson
2012-04-13 18:40 ` [PATCH 10/20] libxl: ao: Convert libxl_run_bootloader Ian Jackson
2012-04-13 18:40 ` [PATCH 11/20] libxl: make libxl_create_logfile const-correct Ian Jackson
2012-04-13 18:40 ` [PATCH 12/20] libxl: log bootloader output Ian Jackson
2012-04-13 18:40 ` [PATCH 13/20] libxl: Allow AO_GC and EGC_GC even if not used Ian Jackson
2012-04-16 14:43 ` Ian Campbell
2012-04-13 18:40 ` [PATCH 14/20] libxl: remove ctx->waitpid_instead Ian Jackson
2012-04-13 18:40 ` [PATCH 15/20] libxl: change some structures to unit arrays Ian Jackson
2012-04-13 18:40 ` [PATCH 16/20] libxl: ao: convert libxl__spawn_* Ian Jackson
2012-04-13 18:40 ` [PATCH 17/20] libxl: make libxl_create run bootloader via callback Ian Jackson
2012-04-13 18:40 ` [PATCH 18/20] libxl: provide progress reporting for long-running operations Ian Jackson
2012-04-13 18:40 ` [PATCH 19/20] libxl: remove malloc failure handling from NEW_EVENT Ian Jackson
2012-04-13 18:40 ` [PATCH 20/20] libxl: convert console callback to libxl_asyncprogress_how Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.