Ian Jackson
2012-May-18 18:25 UTC
[PATCH v2] xl: track child processes for the benefit of libxl
Each time xl forks, it needs to record the pid, so that its exit
status can be preserved if it happens that libxl''s event loop reaped
it. Consequently we also have a new wrapper for waitpid, which in
that case returns the previously-reaped status.
When we get a console ready callback from libxl, check to see if we
have spawned but not reaped a previous console client, and if so reap
it now. (This is necessary to prevent improper use of the xlchild
struct, but has the happy consequence of checking the exit status of
the first console client in the pygrub case.)
Refactor the two calls to libxl_ctx_alloc into a new function
xl_ctx_alloc which also sets the child reaped handler callback.
All of this has the effect of suppressing a message
unknown child [nnnn] unexpected exited status zero
which would sometimes (depending on a race) appear with `xl create -c''
and pygrub.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reported-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
Changes since v1:
* Replace macro-based iteration over children with enum-based version.
* Add comment explaining that xl_waitpid clears the xlchild for reuse.
* Add comment re clearing other children in xl_fork.
* Fix incorrect comment re pid==-1 in xlchild.
* Change local variable "child" in xl_waitpid to "ch" for
consistency.
* Introduce xl_child_pid to hide .pid somewhat more.
* Introduce migration_child local variable in migration_child_report
to simplify error logging.
---
tools/libxl/xl.c | 86 +++++++++++++++++++++++++++++++++++++++-------
tools/libxl/xl.h | 30 +++++++++++++++-
tools/libxl/xl_cmdimpl.c | 63 +++++++++++++++++++---------------
3 files changed, 136 insertions(+), 43 deletions(-)
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 750a61c..5ab9c06 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -102,22 +102,85 @@ void postfork(void)
libxl_postfork_child_noexec(ctx); /* in case we don''t exit/exec */
ctx = 0;
- if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
(xentoollog_logger*)logger)) {
- fprintf(stderr, "cannot reinit xl context after fork\n");
- exit(-1);
- }
+ xl_ctx_alloc();
}
-pid_t xl_fork(libxl_ctx *ctx) {
- pid_t pid;
+pid_t xl_fork(xlchildnum child) {
+ xlchild *ch = &children[child];
+ int i;
+
+ assert(!ch->pid);
+ ch->reaped = 0;
- pid = fork();
- if (pid == -1) {
+ ch->pid = fork();
+ if (ch->pid == -1) {
perror("fork failed");
exit(-1);
}
- return pid;
+ if (!ch->pid) {
+ /* We are in the child now. So all these children are not ours. */
+ for (i=0; i<child_max; i++)
+ children[i].pid = 0;
+ }
+
+ return ch->pid;
+}
+
+pid_t xl_waitpid(xlchildnum child, int *status, int flags)
+{
+ xlchild *ch = &children[child];
+ pid_t got = ch->pid;
+ assert(got);
+ if (ch->reaped) {
+ *status = ch->status;
+ ch->pid = 0;
+ return got;
+ }
+ for (;;) {
+ got = waitpid(ch->pid, status, flags);
+ if (got < 0 && errno == EINTR) continue;
+ if (got > 0) {
+ assert(got == ch->pid);
+ ch->pid = 0;
+ }
+ return got;
+ }
+}
+
+int xl_child_pid(xlchildnum child)
+{
+ xlchild *ch = &children[child];
+ return ch->pid;
+}
+
+static int xl_reaped_callback(pid_t got, int status, void *user)
+{
+ int i;
+ assert(got);
+ for (i=0; i<child_max; i++) {
+ xlchild *ch = &children[i];
+ if (ch->pid == got) {
+ ch->reaped = 1;
+ ch->status = status;
+ return 1;
+ }
+ }
+ return 0;
+}
+
+static const libxl_childproc_hooks childproc_hooks = {
+ .chldowner = libxl_sigchld_owner_libxl,
+ .reaped_callback = xl_reaped_callback,
+};
+
+void xl_ctx_alloc(void) {
+ if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
(xentoollog_logger*)logger)) {
+ fprintf(stderr, "cannot init xl context\n");
+ exit(1);
+ }
+
+ libxl_childproc_setmode(ctx, &childproc_hooks, 0);
}
int main(int argc, char **argv)
@@ -159,10 +222,7 @@ int main(int argc, char **argv)
logger = xtl_createlogger_stdiostream(stderr, minmsglevel, 0);
if (!logger) exit(1);
- if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
(xentoollog_logger*)logger)) {
- fprintf(stderr, "cannot init xl context\n");
- exit(1);
- }
+ xl_ctx_alloc();
/* Read global config file options */
ret = asprintf(&config_file, "%s/xl.conf",
libxl_xen_config_dir_path());
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index b7eacaa..2af9428 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -15,6 +15,8 @@
#ifndef XL_H
#define XL_H
+#include <assert.h>
+
#include "xentoollog.h"
struct cmd_spec {
@@ -108,8 +110,32 @@ struct cmd_spec *cmdtable_lookup(const char *s);
extern libxl_ctx *ctx;
extern xentoollog_logger_stdiostream *logger;
-pid_t xl_fork(libxl_ctx *ctx); /* like fork, but prints and dies if it fails */
-void postfork(void);
+
+void xl_ctx_alloc(void);
+
+/* child processes */
+
+typedef struct {
+ /* every struct like this must be in XLCHILD_LIST */
+ pid_t pid; /* 0: not in use */
+ int reaped; /* valid iff pid!=0 */
+ int status; /* valid iff reaped */
+} xlchild;
+
+typedef enum {
+ child_console, child_waitdaemon, child_migration,
+ child_max
+} xlchildnum;
+
+extern xlchild children[child_max];
+
+pid_t xl_fork(xlchildnum); /* like fork, but prints and dies if it fails */
+void postfork(void); /* needed only if we aren''t going to exec right
away */
+
+/* Handles EINTR. Clears out the xlchild so it can be reused. */
+pid_t xl_waitpid(xlchildnum, int *status, int flags);
+
+int xl_child_pid(xlchildnum); /* returns 0 if child struct is not in use */
/* global options */
extern int autoballoon;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3c55a69..dbe5633 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -17,7 +17,6 @@
#include "libxl_osdeps.h"
#include <stdio.h>
-#include <assert.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
@@ -66,6 +65,8 @@ int logfile = 2;
/* every libxl action in xl uses this same libxl context */
libxl_ctx *ctx;
+xlchild children[child_max];
+
/* when we operate on a domain, it is this one: */
static uint32_t domid;
static const char *common_domname;
@@ -1496,19 +1497,33 @@ static int freemem(libxl_domain_build_info *b_info)
return ERROR_NOMEM;
}
+static void console_child_report(void)
+{
+ if (xl_child_pid(child_console)) {
+ int status;
+ pid_t got = xl_waitpid(child_console, &status, 0);
+ if (got < 0)
+ perror("xl: warning, failed to waitpid for console
child");
+ else if (status)
+ libxl_report_child_exitstatus(ctx, XTL_ERROR, "console
child",
+ xl_child_pid(child_console), status);
+ }
+}
+
static void autoconnect_console(libxl_ctx *ctx_ignored,
libxl_event *ev, void *priv)
{
- pid_t *pid = priv;
uint32_t bldomid = ev->domid;
libxl_event_free(ctx, ev);
- *pid = fork();
- if (*pid < 0) {
+ console_child_report();
+
+ pid_t pid = xl_fork(child_console);
+ if (pid < 0) {
perror("unable to fork xenconsole");
return;
- } else if (*pid > 0)
+ } else if (pid > 0)
return;
postfork();
@@ -1580,7 +1595,6 @@ static int create_domain(struct domain_create *dom_info)
int restore_fd = -1;
int status = 0;
const libxl_asyncprogress_how *autoconnect_console_how;
- pid_t child_console_pid = -1;
struct save_file_header hdr;
int restoring = (restore_file || (migrate_fd >= 0));
@@ -1741,7 +1755,6 @@ start:
libxl_asyncprogress_how autoconnect_console_how_buf;
if ( dom_info->console_autoconnect ) {
autoconnect_console_how_buf.callback = autoconnect_console;
- autoconnect_console_how_buf.for_callback = &child_console_pid;
autoconnect_console_how = &autoconnect_console_how_buf;
}else{
autoconnect_console_how = 0;
@@ -1813,19 +1826,17 @@ start:
pid_t child1, got_child;
int nullfd;
- child1 = xl_fork(ctx);
+ child1 = xl_fork(child_waitdaemon);
if (child1) {
printf("Daemon running with PID %d\n", child1);
for (;;) {
- got_child = waitpid(child1, &status, 0);
+ got_child = xl_waitpid(child_waitdaemon, &status, 0);
if (got_child == child1) break;
assert(got_child == -1);
- if (errno != EINTR) {
- perror("failed to wait for daemonizing child");
- ret = ERROR_FAIL;
- goto out;
- }
+ perror("failed to wait for daemonizing child");
+ ret = ERROR_FAIL;
+ goto out;
}
if (status) {
libxl_report_child_exitstatus(ctx, XTL_ERROR,
@@ -1986,10 +1997,7 @@ out:
free(config_data);
-waitpid_out:
- if (child_console_pid > 0 &&
- waitpid(child_console_pid, &status, 0) < 0 && errno
== EINTR)
- goto waitpid_out;
+ console_child_report();
if (deathw)
libxl_evdisable_domain_death(ctx, deathw);
@@ -2833,7 +2841,7 @@ static pid_t create_migration_child(const char *rune, int
*send_fd,
MUST( libxl_pipe(ctx, sendpipe) );
MUST( libxl_pipe(ctx, recvpipe) );
- child = xl_fork(ctx);
+ child = xl_fork(child_migration);
if (!child) {
dup2(sendpipe[0], 0);
@@ -2877,19 +2885,20 @@ static int migrate_read_fixedmessage(int fd, const void
*msg, int msgsz,
return 0;
}
-static void migration_child_report(pid_t migration_child, int recv_fd) {
+static void migration_child_report(int recv_fd) {
pid_t child;
int status, sr;
struct timeval now, waituntil, timeout;
static const struct timeval pollinterval = { 0, 1000 }; /* 1ms */
- if (!migration_child) return;
+ if (!xl_child_pid(child_migration)) return;
CHK_ERRNO( gettimeofday(&waituntil, 0) );
waituntil.tv_sec += 2;
for (;;) {
- child = waitpid(migration_child, &status, WNOHANG);
+ pid_t migration_child = xl_child_pid(child_migration);
+ child = xl_waitpid(child_migration, &status, WNOHANG);
if (child == migration_child) {
if (status)
@@ -2899,7 +2908,6 @@ static void migration_child_report(pid_t migration_child,
int recv_fd) {
break;
}
if (child == -1) {
- if (errno == EINTR) continue;
fprintf(stderr, "wait for migration child [%ld] failed:
%s\n",
(long)migration_child, strerror(errno));
break;
@@ -2939,7 +2947,6 @@ static void migration_child_report(pid_t migration_child,
int recv_fd) {
}
}
}
- migration_child = 0;
}
static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
@@ -2958,7 +2965,7 @@ static void migrate_do_preamble(int send_fd, int recv_fd,
pid_t child,
"banner", rune);
if (rc) {
close(send_fd);
- migration_child_report(child, recv_fd);
+ migration_child_report(recv_fd);
exit(-rc);
}
@@ -3083,13 +3090,13 @@ static void migrate_domain(const char *domain_spec,
const char *rune,
failed_suspend:
close(send_fd);
- migration_child_report(child, recv_fd);
+ migration_child_report(recv_fd);
fprintf(stderr, "Migration failed, failed to suspend at
sender.\n");
exit(-ERROR_FAIL);
failed_resume:
close(send_fd);
- migration_child_report(child, recv_fd);
+ migration_child_report(recv_fd);
fprintf(stderr, "Migration failed, resuming at sender.\n");
libxl_domain_resume(ctx, domid, 0);
exit(-ERROR_FAIL);
@@ -3104,7 +3111,7 @@ static void migrate_domain(const char *domain_spec, const
char *rune,
" responsibility to avoid that. Sorry.\n");
close(send_fd);
- migration_child_report(child, recv_fd);
+ migration_child_report(recv_fd);
exit(-ERROR_BADFAIL);
}
--
1.7.2.5
Ian Jackson
2012-May-18 18:27 UTC
Re: [PATCH v2] xl: track child processes for the benefit of libxl
Ian Jackson writes ("[PATCH v2] xl: track child processes for the benefit
of libxl"):> Each time xl forks, it needs to record the pid, so that its exit
> status can be preserved if it happens that libxl''s event loop
reaped
> it. Consequently we also have a new wrapper for waitpid, which in
> that case returns the previously-reaped status.
I have tested this on top of current tip ("x86/mem_sharing: Allow
paging-in pages to be replaced by shared ones") plus my event
debugging series and:
Loading new save file incoming migration stream (new xl fmt info 0x0/0x0/853)
Savefile contains xl domain config
xc: Saving memory: iter 0 (last sent 0 skipped 0): 131072/131072 100%
xc: Saving memory: iter 1 (last sent 130814 skipped 258): 131072/131072 100%
xc: Saving memory: iter 2 (last sent 306 skipped 0): 131072/131072 100%
xc: Saving memory: iter 3 (last sent 53 skipped 0): 131072/131072 100%
xc: Saving memory: iter 4 (last sent 0 skipped 0): 131072/131072 100%
xc: error: Max batch size exceeded (-18). Giving up.: Internal error
xc: error: Error when reading batch (90 = Message too long): Internal error
libxl: error: libxl_dom.c:430:libxl__domain_restore_common: restoring domain:
Resource temporarily unavailable
libxl: error: libxl_create.c:596:do_domain_create: cannot (re-)build domain: -3
libxl: error: libxl.c:1043:libxl_domain_destroy: non-existant domain 123
migration target: Domain creation failed (code -3).
libxl: error: libxl_utils.c:364:libxl_read_exactly: file/stream truncated
reading ready message from migration receiver stream
libxl: info: libxl_exec.c:108:libxl_report_child_exitstatus: migration target
process [28059] exited with error status 3
Migration failed, resuming at sender.
root@bedbug:~/shadow/tools/libxl#
I don''t think this is my fault. Also after this happens the guest
crashes:
root@bedbug:~# xl console debian.guest.osstest
[ 47.118283] ------------[ cut here ]------------
[ 47.118283] kernel BUG at
/tmp/buildd/linux-2.6-2.6.32/debian/build/source_i386_none/drivers/xen/events.c:839!
[ 47.118283] invalid opcode: 0000 [#1] SMP
[ 47.118283] last sysfs file: /sys/devices/virtual/net/lo/operstate
[ 47.118283] Modules linked in: evdev snd_pcm snd_timer snd soundcore
snd_page_alloc pcspkr ext3 jbd mbcache xen_netfront xen_blkfront
[ 47.118283]
[ 47.118283] Pid: 531, comm: kstop/0 Not tainted (2.6.32-5-686-bigmem #1)
[ 47.118283] EIP: 0061:[<c1196709>] EFLAGS: 00010082 CPU: 0
[ 47.118283] EIP is at xen_irq_resume+0xd9/0x21f
[ 47.118283] EAX: ffffffef EBX: 00000000 ECX: deadbeef EDX: de2d5f40
[ 47.118283] ESI: 00000000 EDI: de2d5fb0 EBP: c14f29fc ESP: de2d5f10
[ 47.118283] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0069
[ 47.118283] Process kstop/0 (pid: 531, ti=de2d4000 task=df968000
task.ti=de2d4000)
[ 47.118283] Stack:
[ 47.118283] 00000000 00000003 c143b474 c24184f0 00000000 00000000 00000002
00000010
[ 47.118283] <0> c1005744 deadbeef 00000003 de2d5fb0 00000000 00000000
0000f3f3 deadbeef
[ 47.118283] <0> 00000003 de2d5fb0 00000000 c1197494 df851f8c c14995ec
00000003 de2d5fb0
[ 47.118283] Call Trace:
[ 47.118283] [<c1005744>] ? xen_force_evtchn_callback+0xc/0x10
[ 47.118283] [<c1197494>] ? xen_suspend+0x99/0xb0
[ 47.118283] [<c106abd0>] ? stop_cpu+0x6d/0xab
[ 47.118283] [<c1047c37>] ? worker_thread+0x141/0x1bd
[ 47.118283] [<c106ab63>] ? stop_cpu+0x0/0xab
[ 47.118283] [<c104a97a>] ? autoremove_wake_function+0x0/0x2d
[ 47.118283] [<c1047af6>] ? worker_thread+0x0/0x1bd
[ 47.118283] [<c104a748>] ? kthread+0x61/0x66
[ 47.118283] [<c104a6e7>] ? kthread+0x0/0x66
[ 47.118283] [<c1008d87>] ? kernel_thread_helper+0x7/0x10
[ 47.118283] Code: fe 0f b7 45 08 39 d8 74 04 0f 0b eb fe 8b 44 24 10 8d 54 24
30 89 5c 24 30 89 44 24 34 b8 01 00 00 00 e8 f3 fa ff ff 85 c0 74 04 <0f>
0b eb fe 8b 4c 24 38 8d 7c 24 24 89 0c 24 89 34 8d e0 1e 3c
[ 47.118283] EIP: [<c1196709>] xen_irq_resume+0xd9/0x21f SS:ESP
0069:de2d5f10
[ 47.118283] ---[ end trace 838e9608136663d9 ]---
[ 47.118283] ------------[ cut here ]------------
[ 47.118283] WARNING: at
/tmp/buildd/linux-2.6-2.6.32/debian/build/source_i386_none/kernel/time/timekeeping.c:260
ktime_get+0x1f/0xcb()
[ 47.118283] Modules linked in: evdev snd_pcm snd_timer snd soundcore
snd_page_alloc pcspkr ext3 jbd mbcache xen_netfront xen_blkfront
[ 47.118283] Pid: 0, comm: swapper Tainted: G D 2.6.32-5-686-bigmem #1
[ 47.118283] Call Trace:
[ 47.118283] [<c1051b1c>] ? ktime_get+0x1f/0xcb
[ 47.118283] [<c1051b1c>] ? ktime_get+0x1f/0xcb
[ 47.118283] [<c1036ac5>] ? warn_slowpath_common+0x5e/0x8a
[ 47.118283] [<c1036afb>] ? warn_slowpath_null+0xa/0xc
[ 47.118283] [<c1051b1c>] ? ktime_get+0x1f/0xcb
[ 47.118283] [<c1055bc8>] ? tick_nohz_stop_sched_tick+0x5d/0x36e
[ 47.118283] [<c104d83d>] ? hrtimer_start_range_ns+0xf/0x13
[ 47.118283] [<c1055b66>] ? tick_nohz_restart_sched_tick+0x128/0x12d
[ 47.118283] [<c1007395>] ? cpu_idle+0x67/0xa2
[ 47.118283] [<c13dc80d>] ? start_kernel+0x318/0x31d
[ 47.118283] [<c13de2eb>] ? xen_start_kernel+0x46a/0x471
[ 47.118283] [<c1409043>] ? udp_init+0x30/0x8c
[ 47.118283] ---[ end trace 838e9608136663da ]---
Ian Campbell
2012-May-22 08:24 UTC
Re: [PATCH v2] xl: track child processes for the benefit of libxl
On Fri, 2012-05-18 at 19:25 +0100, Ian Jackson wrote:> Each time xl forks, it needs to record the pid, so that its exit > status can be preserved if it happens that libxl''s event loop reaped > it. Consequently we also have a new wrapper for waitpid, which in > that case returns the previously-reaped status. > > When we get a console ready callback from libxl, check to see if we > have spawned but not reaped a previous console client, and if so reap > it now. (This is necessary to prevent improper use of the xlchild > struct, but has the happy consequence of checking the exit status of > the first console client in the pygrub case.) > > Refactor the two calls to libxl_ctx_alloc into a new function > xl_ctx_alloc which also sets the child reaped handler callback. > > All of this has the effect of suppressing a message > unknown child [nnnn] unexpected exited status zero > which would sometimes (depending on a race) appear with `xl create -c'' > and pygrub. > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > Reported-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Roger Pau Monne <roger.pau@citrix.com> > > Changes since v1: > * Replace macro-based iteration over children with enum-based version.I find this a lot easier to follow, thanks!> +pid_t xl_waitpid(xlchildnum child, int *status, int flags) > +{ > + xlchild *ch = &children[child]; > + pid_t got = ch->pid; > + assert(got); > + if (ch->reaped) { > + *status = ch->status; > + ch->pid = 0; > + return got; > + } > + for (;;) { > + got = waitpid(ch->pid, status, flags);Is it always the case that xl has at most one child? Because if not then we may reap the wrong one here. I believe we do never have more than one child, the corner case would be if we have a console child and a daemon child simultaneously, but I don''t think that can happen (likewise migration child and either of the others).> @@ -108,8 +110,32 @@ struct cmd_spec *cmdtable_lookup(const char *s); > > extern libxl_ctx *ctx; > extern xentoollog_logger_stdiostream *logger; > -pid_t xl_fork(libxl_ctx *ctx); /* like fork, but prints and dies if it fails */ > -void postfork(void); > + > +void xl_ctx_alloc(void); > + > +/* child processes */ > + > +typedef struct { > + /* every struct like this must be in XLCHILD_LIST */This comment is obsolete now. [...]
Ian Jackson
2012-May-22 11:17 UTC
Re: [PATCH v2] xl: track child processes for the benefit of libxl
Ian Campbell writes ("Re: [PATCH v2] xl: track child processes for the
benefit of libxl"):> On Fri, 2012-05-18 at 19:25 +0100, Ian Jackson wrote:
> > +pid_t xl_waitpid(xlchildnum child, int *status, int flags)
> > +{
> > + xlchild *ch = &children[child];
> > + pid_t got = ch->pid;
> > + assert(got);
> > + if (ch->reaped) {
> > + *status = ch->status;
> > + ch->pid = 0;
> > + return got;
> > + }
> > + for (;;) {
> > + got = waitpid(ch->pid, status, flags);
>
> Is it always the case that xl has at most one child?
I don''t think it is necessarily the case.
> Because if not then we may reap the wrong one here.
No, the whole point of waitpid is that you get to specify which child
to reap. Unless I''m missing something ?
ch->pid the same as children[child].pid which is the pid of the child
we want to wait for, and the previous code has just checked whether we
have reaped it already so we are guaranteed not to reap a new
different child with the same pid and get confused (should such a
thing happen to happen).
> > +typedef struct {
> > + /* every struct like this must be in XLCHILD_LIST */
>
> This comment is obsolete now.
Oh yes.
Ian.
Ian Campbell
2012-May-22 12:13 UTC
Re: [PATCH v2] xl: track child processes for the benefit of libxl
On Tue, 2012-05-22 at 12:17 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [PATCH v2] xl: track child processes for the benefit of libxl"): > > On Fri, 2012-05-18 at 19:25 +0100, Ian Jackson wrote: > > > +pid_t xl_waitpid(xlchildnum child, int *status, int flags) > > > +{ > > > + xlchild *ch = &children[child]; > > > + pid_t got = ch->pid; > > > + assert(got); > > > + if (ch->reaped) { > > > + *status = ch->status; > > > + ch->pid = 0; > > > + return got; > > > + } > > > + for (;;) { > > > + got = waitpid(ch->pid, status, flags); > > > > Is it always the case that xl has at most one child? > > I don''t think it is necessarily the case. > > > Because if not then we may reap the wrong one here. > > No, the whole point of waitpid is that you get to specify which child > to reap. Unless I''m missing something ?No I was, for some reason I thought we were waiting for any child here. You can safely ignore this comment ;-) Ian.
Ian Campbell
2012-May-25 09:37 UTC
Re: [PATCH v2] xl: track child processes for the benefit of libxl
On Fri, 2012-05-18 at 19:25 +0100, Ian Jackson wrote:> Each time xl forks, it needs to record the pid, so that its exitI was just about to apply this but testing with pygrub gives me: libxl: critical: libxl_fork.c:314:libxl__fork_selfpipe_woken: DISASTER in event loop: reported by libxl_childproc_hooks->reaped_callback (for pid=7023, status=0; error code 1): Success libxl: critical: libxl_event.c:1005:libxl__event_disaster: DISASTER in event loop not handled by libxl application libxl: fatal error, exiting program: DISASTER in event loop not handled by libxl application I think the return codes of xl_reaped_callback are just bogus, according to the comment it should return 0 if it knows the child, ERROR_UNKNOWN_CHILD if not and anything else is a disaster. Incremental fix is: diff -r a5ff801ed897 tools/libxl/xl.c --- a/tools/libxl/xl.c Fri May 25 10:32:06 2012 +0100 +++ b/tools/libxl/xl.c Fri May 25 10:37:37 2012 +0100 @@ -163,10 +163,10 @@ static int xl_reaped_callback(pid_t got, if (ch->pid == got) { ch->reaped = 1; ch->status = status; - return 1; + return 0; } } - return 0; + return ERROR_UNKNOWN_CHILD; } static const libxl_childproc_hooks childproc_hooks = {
Ian Jackson
2012-May-25 10:58 UTC
Re: [PATCH v2] xl: track child processes for the benefit of libxl
Ian Campbell writes ("Re: [PATCH v2] xl: track child processes for the
benefit of libxl"):> On Fri, 2012-05-18 at 19:25 +0100, Ian Jackson wrote:
> > Each time xl forks, it needs to record the pid, so that its exit
>
> I was just about to apply this but testing with pygrub gives me:
...> I think the return codes of xl_reaped_callback are just bogus, according
> to the comment it should return 0 if it knows the child,
> ERROR_UNKNOWN_CHILD if not and anything else is a disaster. Incremental
> fix is:
Ooops, I''m sorry. I broke this when I removed the CHILD_LIST macro
stuff.
Ian.
Ian Campbell
2012-May-29 09:08 UTC
Re: [PATCH v2] xl: track child processes for the benefit of libxl
On Fri, 2012-05-25 at 11:58 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [PATCH v2] xl: track child processes for the benefit of libxl"): > > On Fri, 2012-05-18 at 19:25 +0100, Ian Jackson wrote: > > > Each time xl forks, it needs to record the pid, so that its exit > > > > I was just about to apply this but testing with pygrub gives me: > ... > > I think the return codes of xl_reaped_callback are just bogus, according > > to the comment it should return 0 if it knows the child, > > ERROR_UNKNOWN_CHILD if not and anything else is a disaster. Incremental > > fix is: > > Ooops, I''m sorry. I broke this when I removed the CHILD_LIST macro > stuff.No problem, I''m going to fold in my incremental fix, ack and apply. Ian.