|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 20/20] libxl: ao: Convert libxl_run_bootloader
On Fri, 2012-03-16 at 16:26 +0000, Ian Jackson wrote:
> 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.
It's always annoying when diff fails to notice this and treats any blank
line as a common point. I've stripped most of the - lines out to make
something easier to review.
>
> Significant changes include:
>
> - We provide and use a new asynchronous internal facility for pty
> creation, which runs openpty in a child for SIGCHLD etc. safety
> (see the comment at the top of libxl__openptys).
>
> - 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.
>
> - This is implemented using a new asynchronous internal data-
> copying facility.
>
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> ---
> tools/libxl/libxl.c | 4 +
> tools/libxl/libxl.h | 3 +-
> tools/libxl/libxl_bootloader.c | 957
> ++++++++++++++++++++++++++--------------
> tools/libxl/libxl_create.c | 8 +-
> tools/libxl/libxl_internal.h | 97 ++++
> 5 files changed, 733 insertions(+), 336 deletions(-)
> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index 2774062..725fa09 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -15,70 +15,219 @@
> #include "libxl_osdeps.h" /* must come before any other headers */
>
> #include <termios.h>
> +#include <utmp.h>
>
> #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);
> +
> +/*----- 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.
> + */
> +
> +/* 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);
> + EGC_GC;
> +
> + 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__gc *gc, libxl__openpty_state *op,
> + const struct termios *termp,
> + const struct winsize *winp) {
"{" on next line please, (here and elsewhere)
libxl__openptys might be deserving of its own home outside
libxl_bootloader.c?
> + /*
> + * 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.
> + */
> + 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);
this is sockets[1], which we then read below (via recvmsg_fds)? I
suspect one or the other is wrong (I think the read).
> + for_child = 0;
> +
> + /* this should be fast so do it synchronously */
> +
> + libxl__carefd_begin();
> + char buf[1];
> + rc = libxl__recvmsg_fds(gc, sockets[1], 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]);
> + if (sockets[1] >= 0) close(sockets[1]);
> + libxl__carefd_close(for_child);
for_child == sockets[1], so now we've closed it twice.
> + 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;
> +}
> +
> +/*----- bootloader arguments -----*/
> +
> +static void bootloader_arg(libxl__bootloader_state *bl, const char *arg) {
> + assert(bl->nargs < bl->argsspace);
> + bl->args[bl->nargs++] = arg;
> +}
I'm not a huge fan of the flexarray stuff but is reinventing them again
useful?
> +
> +static void make_bootloader_args(libxl__gc *gc, libxl__bootloader_state *bl)
> {
> + const libxl_domain_build_info *info = bl->info;
> +
> + bl->argsspace = 7 +
> libxl_string_list_length(&info->u.pv.bootloader_args);
> +
> + 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", "/var/run/xen/"));
Output path changed? If we are going to do that then perhaps it should
include an additional element from mkdtemp()?
(comes back later.. why not use XEN_RUN_DIR? Also you seem to have
created bl->outputdir for this purpose anyway?)
> 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 */
Pre-existing:
Sentinel
> - flexarray_set(args, nr++, NULL);
> + ARG(NULL);
>
> - return (char **) flexarray_contents(args); /* Frees args */
Hopefully I'll find blargs getting free'd later on...
> +#undef ARG
> }
>
> +/*----- synchronous subroutines -----*/
> +
> +static int setup_xenconsoled_pty(libxl__egc *egc, libxl__bootloader_state
> *bl,
> + char *slave_path, size_t slave_path_len)
> {
> + EGC_GC;
> struct termios termattr;
> + 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;
> }
>
> /*
> @@ -92,309 +241,360 @@ static int open_xenconsoled_pty(int *master, int
> *slave, char *slave_path, size_
> * for it.
> */
> #if !defined(__sun__) && !defined(__NetBSD__)
> + tcgetattr(master, &termattr);
> cfmakeraw(&termattr);
> + tcsetattr(master, TCSANOW, &termattr);
>
> + close(slave);
> + slave = -1;
> #else
> + tcgetattr(slave, &termattr);
> cfmakeraw(&termattr);
> + tcsetattr(slave, TCSANOW, &termattr);
> #endif
Could this stuff be usefully pushed into libxl__openpty?
>
> - fcntl(*master, F_SETFL, O_NDELAY);
Why not?
> - fcntl(*master, F_SETFD, FD_CLOEXEC);
This one I understand (done by carefd stuff already)
> return 0;
[...]
> + out:
> + return rc;
> +}
>
> - /*
> - * 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
I think we are right to give up any pretence of supporting Solaris now.
[....]
> +/*----- data copier -----*/
> +static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state
> *dc)
> +{
> + EGC_GC;
> + 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"
> + " during copy (%s)", dc->what);
> + datacopier_callback(egc, dc, -1, 0);
> + return;
> + }
> }
[...]
> + } else if (!libxl__ev_fd_isregistered(&dc->toread)) {
> + /* we have had eof */
> + libxl__datacopier_kill(gc, dc);
> + dc->callback(egc, dc, 0, 0);
> + return;
> + } else {
> + /* nothing buffered, but still reading */
> + libxl__ev_fd_deregister(gc, &dc->towrite);
is it worth the effort to handle only registering for write events when
something is buffered? It would be simpler to just set it up at start of
day and leave it until done?
> + }
> +}
> +
> +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);
> + EGC_GC;
> +
> + if (revents & ~POLLIN) {
> + LOG(ERROR, "unexpected poll event 0x%x (should be POLLIN)"
> + " during copy (%s)", revents, dc->what);
> + datacopier_callback(egc, dc, -1, 0);
> + return;
> + }
> + assert(revents & POLLIN);
> + for (;;) {
> + while (dc->used >= dc->maxsz) {
This is the overflow discard loop, correct? (took me a while to figure
that out, in the meantime I was v. confused ;-))
> + 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);
> }
>
[...]
> +/*----- 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;
Not -1? A more typical "invalid" fd value.
> + 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)
> +{
> + libxl__ao *ao = bl->ao;
> + AO_GC;
> + int i;
> +
> + if (bl->diskpath) {
> + libxl_device_disk_local_detach(CTX, bl->disk);
> + free(bl->diskpath);
> + bl->diskpath = 0;
> + }
> + libxl__datacopier_kill(gc, &bl->keystrokes);
> + libxl__datacopier_kill(gc, &bl->display);
> + for (i=0; i<2; i++) {
> + libxl__carefd_close(bl->ptys[0].master);
> + libxl__carefd_close(bl->ptys[0].slave);
Don't you mean [i] rather than [0] both times here?
> }
> }
[...]
> +/*----- main flow of control -----*/
>
> +void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
> +{
> + libxl__ao *ao = bl->ao;
> + AO_GC;
> + libxl_domain_build_info *info = bl->info;
> + uint32_t domid = bl->domid;
> + int rc, r;
>
> + 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;
> + }
>
[...]
> + bl->outputdir = GCSPRINTF(XEN_RUN_DIR "/bootloader.%"PRIu32".d", domid);
> + bl->outputpath = GCSPRINTF(XEN_RUN_DIR "/bootloader.%"PRIu32".out",
> domid);
You should use this instead of the hardcoded /var/run/xen way above?
>
[...]
> + 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;
> + }
>
[...]
> + make_bootloader_args(gc, bl);
>
[...]
> + bl->diskpath = libxl_device_disk_local_attach(CTX, bl->disk);
> + if (!bl->diskpath) {
> + rc = ERROR_FAIL;
> goto out;
> + }
>
[...]
> + bl->openpty.callback = bootloader_gotptys;
> + bl->openpty.count = 2;
> + bl->openpty.results = bl->ptys;
> + rc = libxl__openptys(gc, &bl->openpty, 0,0);
> + if (rc) goto out;
>
[...]
> + return;
>
[...]
> + out:
> + assert(rc);
> + bootloader_callback(egc, bl, rc);
> +}
>
[...]
> +static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
> +{
> + libxl__bootloader_state *bl = CONTAINER_OF(op, *bl, openpty);
> + EGC_GC;
> + int rc, r;
>
[...]
> + if (bl->openpty.rc) {
> + rc = bl->openpty.rc;
> + goto out;
> }
>
> /*
> @@ -407,76 +607,167 @@ 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.
> */
[...]
> +
> + 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 (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;
> }
>
[...]
> + 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.readfd = xenconsole_master;
> + bl->keystrokes.writefd = bootloader_master;
> + bl->keystrokes.maxsz = BOOTLOADER_BUF_OUT;
> + bl->keystrokes.what = GCSPRINTF("bootloader input for %"PRIu32,
> bl->domid);
> + bl->keystrokes.callback = bootloader_keystrokes_copyfail;
> + rc = libxl__datacopier_start(gc, &bl->keystrokes);
> + if (rc) goto out;
>
[...]
> + bl->display.readfd = bootloader_master;
> + bl->display.writefd = xenconsole_master;
> + bl->display.maxsz = BOOTLOADER_BUF_IN;
> + bl->display.what = GCSPRINTF("bootloader output for %"PRIu32, bl->domid);
> + bl->display.callback = bootloader_display_copyfail;
> + rc = libxl__datacopier_start(gc, &bl->display);
> + if (rc) goto out;
>
[...]
> + struct termios termattr;
>
[...]
> + pid_t pid = libxl__ev_child_fork(gc, &bl->child, bootloader_finished);
> + if (pid == -1) {
> + rc = ERROR_FAIL;
> + goto out;
> }
>
[...]
> + if (!pid) {
> + /* child */
> + r = login_tty(libxl__carefd_fd(bl->ptys[0].slave));
login_tty is a new one to me. When I lookup up the manpage I happened to
notice:
Link with -lutil.
> + 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);
> }
>
[...]
> + /* parent */
> + libxl__carefd_close(bl->ptys[0].slave);
> + bl->ptys[0].slave = 0;
>
[...]
> + /*
> + * On Solaris, the master pty side does not have terminal semantics,
> + * so don't try to set any attributes, as it will fail.
> + */
/insert comment about supporting Solaris again here too...
> +#if !defined(__sun__)
> + tcgetattr(bootloader_master, &termattr);
> + cfmakeraw(&termattr);
> + tcsetattr(bootloader_master, TCSANOW, &termattr);
> +#endif
> +
> + return;
> +
> + out:
> + bootloader_callback(egc, bl, rc);
> +}
> +
[...]
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index d486af2..5b73db8 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>
> @@ -1433,6 +1434,102 @@ 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 happend when writing, logged, errno is valid
> + * onwrite==0 means failure happend when reading
happened (twice)
[...]
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |