|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH 3/3] libxl: event system: properly register the SIGCHLD self-pipe
An application which uses libxl_osevent_register_hooks, and doesn't
use libxl_sigchld_owner_mainloop, would never properly experience the
deaths of its (libxl) children.
This is because the SIGCHLD self pipe would be handled using ad-hoc
code in beforepoll_internal and afterpoll_internal. However, this is
no good if application is using the fd registration system instead; in
that case these functions would not be called and nothing would deal
with readability of the self pipe.
Fix this as follows:
The SIGCHLD self pipe now is now dealt with by a standard libxl__ev_fd
handler, which is registered and deregistered along with the SIGCHLD
handler itself. The handler function subsumes the ad-hoc response
code removed from beforepoll_internal, and the functionality of
libxl__fork_selfpipe_woken.
This is also tidier as the SIGCHLD self pipe is no longer touched
outside libxl_fork.c other than in ctx initialisation and teardown.
(The ad-hoc arrangements for poller->wakeup_pipe in
beforepoll_internal and afterpoll_internal are OK because the
libxl__poller mechanism exists to wake up threads which are sitting
inside libxl's poll loop, so is not applicable to the application's
event loop.)
Reported-by: Bamvor Jian Zhang <bjzhang@xxxxxxxx>
Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Cc: Bamvor Jian Zhang <bjzhang@xxxxxxxx>
Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
Cc: Jim Fehlig <jfehlig@xxxxxxxx>
---
tools/libxl/libxl.c | 2 ++
tools/libxl/libxl_event.c | 12 ----------
tools/libxl/libxl_fork.c | 50 ++++++++++++++++++++++++++++++++----------
tools/libxl/libxl_internal.h | 3 +--
4 files changed, 42 insertions(+), 25 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2ebba98..3efc564 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -65,6 +65,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
ctx->childproc_user = 0;
ctx->sigchld_selfpipe[0] = -1;
+ libxl__ev_fd_init(&ctx->sigchld_selfpipe_efd);
/* The mutex is special because we can't idempotently destroy it */
@@ -146,6 +147,7 @@ int libxl_ctx_free(libxl_ctx *ctx)
for (i = 0; i < ctx->watch_nslots; i++)
assert(!libxl__watch_slot_contents(gc, i));
libxl__ev_fd_deregister(gc, &ctx->watch_efd);
+ libxl__ev_fd_deregister(gc, &ctx->sigchld_selfpipe_efd);
/* Now there should be no more events requested from the application: */
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 54d15db..bdef7ac 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -774,10 +774,6 @@ 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{ \
@@ -999,14 +995,6 @@ static void afterpoll_internal(libxl__egc *egc,
libxl__poller *poller,
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 (;;) {
libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes);
if (!etime)
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index a581763..4ae9f94 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -155,6 +155,9 @@ int libxl__carefd_fd(const libxl__carefd *cf)
* Actual child process handling
*/
+static void sigchld_selfpipe_handler(libxl__egc *egc, libxl__ev_fd *ev,
+ int fd, short events, short revents);
+
static void sigchld_handler(int signo)
{
int esave = errno;
@@ -177,10 +180,18 @@ static void sigchld_removehandler_core(void)
void libxl__sigchld_removehandler(libxl__gc *gc) /* non-reentrant */
{
+ int rc;
+
atfork_lock();
if (sigchld_owner == CTX)
sigchld_removehandler_core();
atfork_unlock();
+
+ if (libxl__ev_fd_isregistered(&CTX->sigchld_selfpipe_efd)) {
+ rc = libxl__ev_fd_modify(gc, &CTX->sigchld_selfpipe_efd, 0);
+ if (rc)
+ libxl__ev_fd_deregister(gc, &CTX->sigchld_selfpipe_efd);
+ }
}
int libxl__sigchld_installhandler(libxl__gc *gc) /* non-reentrant */
@@ -197,6 +208,15 @@ int libxl__sigchld_installhandler(libxl__gc *gc) /*
non-reentrant */
goto out;
}
}
+ if (!libxl__ev_fd_isregistered(&CTX->sigchld_selfpipe_efd)) {
+ rc = libxl__ev_fd_register(gc, &CTX->sigchld_selfpipe_efd,
+ sigchld_selfpipe_handler,
+ CTX->sigchld_selfpipe[0], POLLIN);
+ if (rc) goto out;
+ } else {
+ rc = libxl__ev_fd_modify(gc, &CTX->sigchld_selfpipe_efd, POLLIN);
+ if (rc) goto out;
+ }
atfork_lock();
if (sigchld_owner != CTX) {
@@ -237,15 +257,6 @@ static bool chldmode_ours(libxl_ctx *ctx, bool creating)
abort();
}
-int libxl__fork_selfpipe_active(libxl_ctx *ctx)
-{
- /* Returns the fd to read, or -1 */
- if (!chldmode_ours(ctx, 0))
- return -1;
-
- return ctx->sigchld_selfpipe[0];
-}
-
static void perhaps_removehandler(libxl__gc *gc)
{
if (!chldmode_ours(CTX, 0))
@@ -295,12 +306,29 @@ int libxl_childproc_reaped(libxl_ctx *ctx, pid_t pid, int
status)
return rc;
}
-void libxl__fork_selfpipe_woken(libxl__egc *egc)
+static void sigchld_selfpipe_handler(libxl__egc *egc, libxl__ev_fd *ev,
+ int fd, short events, short revents)
{
/* May make callbacks into the application for child processes.
- * ctx must be locked EXACTLY ONCE */
+ * So, this function may unlock and relock the CTX. This is OK
+ * because event callback functions are always called with the CTX
+ * locked exactly once, and from code which copes with reentrancy.
+ * (See also the comment in afterpoll_internal.) */
EGC_GC;
+ int selfpipe = CTX->sigchld_selfpipe[0];
+
+ if (revents & ~POLLIN) {
+ LOG(ERROR, "unexpected poll event 0x%x on SIGCHLD self pipe", revents);
+ LIBXL__EVENT_DISASTER(egc,
+ "unexpected poll event on SIGCHLD self pipe",
+ 0, 0);
+ }
+ assert(revents & POLLIN);
+
+ int e = libxl__self_pipe_eatall(selfpipe);
+ if (e) LIBXL__EVENT_DISASTER(egc, "read sigchld pipe", e, 0);
+
while (chldmode_ours(CTX, 0) /* in case the app changes the mode */) {
int status;
pid_t pid = waitpid(-1, &status, WNOHANG);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d567a10..b1f5f81 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -353,6 +353,7 @@ struct libxl__ctx {
const libxl_childproc_hooks *childproc_hooks;
void *childproc_user;
int sigchld_selfpipe[2]; /* [0]==-1 means handler not installed */
+ libxl__ev_fd sigchld_selfpipe_efd;
LIBXL_LIST_HEAD(, libxl__ev_child) children;
libxl_version_info version_info;
@@ -840,8 +841,6 @@ _hidden void libxl__poller_wakeup(libxl__egc *egc,
libxl__poller *p);
extern const libxl_childproc_hooks libxl__childproc_default_hooks;
int libxl__sigchld_installhandler(libxl__gc*); /* non-reentrant; logs errs */
void libxl__sigchld_removehandler(libxl__gc*); /* 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 */
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |