On Tue, Jul 01, 2008 at 04:21:38PM +0100, John Levon wrote:

See implementation here:

http://cr.opensolaris.org/~johnlev/virt-console/

(inside libvirt.hg/patches/libvirt/virt-console)

This splits virsh console into a separate binary to allow it to be setuid-root on Solaris (where we check permissions then drop privilege). It also fixes a number of RFEs

--- libvirt-0.4.0/src/Makefile.in 2008-06-27 06:17:20.865621099 -0700 +++ libvirt-1/src/Makefile.in 2008-06-27 06:19:55.983265305 -0700

You can exclude Makefile.in from patches, since its auto-generated.

--- libvirt-0.4.0/src/virsh.c 2008-06-27 13:30:32.395824295 -0700 +++ libvirt-new/src/virsh.c 2008-06-27 13:29:26.021985284 -0700 @@ -48,7 +48,7 @@ #endif

#include "internal.h" -#include "console.h" +#include "util.h"

static char *progname; static int sigpipe; @@ -446,7 +446,7 @@ cmdConnect(vshControl * ctl, vshCmd * cm * "console" command */ static vshCmdInfo info_console[] = { - {"syntax", "console <domain>"}, + {"syntax", "console [--verbose] <domain>"}, {"help", gettext_noop("connect to the guest console")}, {"desc", gettext_noop("Connect the virtual serial console for the guest")}, @@ -454,6 +454,7 @@ static vshCmdInfo info_console[] = { };

static vshCmdOptDef opts_console[] = { + {"verbose", VSH_OT_BOOL, 0, gettext_noop("verbose console")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, {NULL, 0, 0, NULL} }; @@ -463,50 +464,37 @@ static vshCmdOptDef opts_console[] = { static int cmdConsole(vshControl * ctl, vshCmd * cmd) { - xmlDocPtr xml = NULL; - xmlXPathObjectPtr obj = NULL; - xmlXPathContextPtr ctxt = NULL; - virDomainPtr dom; + int verbose = vshCommandOptBool(cmd, "verbose"); + char *argv[] = { "/usr/bin/virt-console", NULL, NULL, NULL, NULL };

This should probably be char *argv[] = { BINDIR "/virt-console", NULL, NULL, NULL, NULL }; so that it auto-adjusts when people pass --prefix to configure

- obj = xmlXPathEval(BAD_CAST "string(/domain/devices/console/@tty)", ctxt); - if ((obj != NULL) && ((obj->type == XPATH_STRING) && - (obj->stringval != NULL) && (obj->stringval[0] != 0))) { - if (vshRunConsole((const char *)obj->stringval) == 0) - ret = TRUE; - } else { - vshPrintExtra(ctl, _("No console available for domain\n")); + ret = virExecNoPipe(ctl->conn, argv, &pid); + if (ret == -1) { + vshError(ctl, FALSE, _("Couldn't execute /usr/bin/virt-console.")); + return FALSE; } - xmlXPathFreeObject(obj);

- cleanup: - if (ctxt) - xmlXPathFreeContext(ctxt); - if (xml) - xmlFreeDoc(xml); - virDomainFree(dom); - return ret; + waitpid(pid, NULL, 0);

Ought to do this in a while loop to handle EINTR.

--- /dev/null 2008-06-30 17:03:12.000000000 -0700 +++ libvirt-new/src/virt-console.c 2008-06-30 16:58:36.079071463 -0700

I'd prefer to keep the source in the 'console.c' file instead of renaming it, just to make historical diff tracking a little easier.

+#include "internal.h" + +/* ie Ctrl-] as per telnet */ +#define CTRL_CLOSE_BRACKET '\35' + +static int got_signal; +static int verbose; +static const char *dom_name; +static const char *conn_name; + +static void +do_signal(int sig ATTRIBUTE_UNUSED) +{ + got_signal = 1; +} + +#ifndef HAVE_CFMAKERAW +static void +cfmakeraw(struct termios *attr) +{ + attr->c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP + | INLCR | IGNCR | ICRNL | IXON); + attr->c_oflag &= ~OPOST; + attr->c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN); + attr->c_cflag &= ~(CSIZE | PARENB); + attr->c_cflag |= CS8; + +#ifdef __sun + attr->c_cc[VMIN] = 0; + attr->c_cc[VTIME] = 0; +#endif +} +#endif /* !HAVE_CFMAKERAW */

A question for Jim here ... does gnulib have a cfmakeraw() implementation we can use ?

+static int +get_domain(virConnectPtr *conn, virDomainPtr *dom, + virDomainInfo *info, int lookup_by_id) +{ + int ret = 0; + int id; + + __priv_bracket(PRIV_ON); + + *conn = virConnectOpenAuth(conn_name, virConnectAuthPtrDefault, 0);

We ought to pass VIR_CONNECT_RO as the 3rd arg here, since the console doesn't require write access.

+ if (*conn == NULL) { + fprintf(stderr, _("Failed to connect to the hypervisor")); + exit(EXIT_FAILURE); + } + + *dom = virDomainLookupByName(*conn, dom_name); + + if (*dom == NULL) + *dom = virDomainLookupByUUIDString(*conn, dom_name); + if (*dom == NULL && lookup_by_id && + xstrtol_i(dom_name, NULL, 10, &id) == 0 && id >= 0)

Can use virStrToLong_i from util.h here.

+ obj = xmlXPathEval(BAD_CAST "string(/domain/devices/console/@tty)", ctxt);

The virXPathString() method from xml.h will simplify the following handling

+ if (obj == NULL) + goto cleanup; + if (obj->type != XPATH_STRING) + goto cleanup; + if (!obj->stringval || obj->stringval[0] == '\0') + goto cleanup; + + tty = strdup((const char *)obj->stringval); + +cleanup: + if (obj != NULL) + xmlXPathFreeObject(obj); + free(doc); + if (ctxt != NULL) + xmlXPathFreeContext(ctxt); + if (xml != NULL) + xmlFreeDoc(xml); + return tty; +}

+static int +check_for_reboot(void) +{ + virConnectPtr conn = NULL; + virDomainPtr dom = NULL; + virDomainInfo info; + int tries = 0; + int ret = 0; + +retry: + if (dom != NULL) + put_domain(conn, dom); + + /* + * Domain ID will vary across reboot, so don't lookup by a given ID. + */ + if (!get_domain(&conn, &dom, &info, 0)) + return 0; + + switch (info.state) { + case VIR_DOMAIN_RUNNING: + case VIR_DOMAIN_BLOCKED: + case VIR_DOMAIN_PAUSED: + ret = 1; + goto out; + break; + + case VIR_DOMAIN_CRASHED: + if (verbose) + fprintf(stderr, _("Domain \"%s\" has crashed."), dom_name); + goto out; + break; + + case VIR_DOMAIN_NOSTATE: + default: + break; + + case VIR_DOMAIN_SHUTDOWN: + if (verbose) + fprintf(stderr, _("Domain \"%s\" is shutting down.\n"), dom_name); + tries = 0; + break; + + case VIR_DOMAIN_SHUTOFF: + if (verbose) + fprintf(stderr, _("Domain \"%s\" has shut down."), dom_name); + goto out; + break; + } + + tries++; + if (tries == 1) { + goto retry; + } else if (tries == 2) { + sleep(1); + goto retry; + }

I think we probably need to wait a little longer than this - 5 times with a 1 second sleep perhaps. Under heavy load it can take a while for reboot to complete

+ +out: + put_domain(conn, dom); + return ret; +} + +static int +open_tty(void) +{ + char *tty; + int ttyfd; + + if ((tty = get_domain_tty()) == NULL) { + if (domain_is_running() != 1) { + fprintf(stderr, _("Domain \"%s\" is not running.\n"), dom_name); + exit(EXIT_FAILURE); + } + + fprintf(stderr, + _("Couldn't get console for domain \"%s\"\n"), dom_name); + exit(EXIT_FAILURE); + } + + __priv_bracket(PRIV_ON); + if ((ttyfd = open(tty, O_NOCTTY | O_RDWR)) < 0) { + fprintf(stderr, _("Unable to open tty %s: %s\n"), + tty, strerror(errno)); + exit(EXIT_FAILURE); + } + + if (lockf(ttyfd, F_TLOCK, 0) == -1) {

Ahh good idea to lock the tty.

+ if (errno == EACCES || errno == EAGAIN) { + fprintf(stderr, + _("Console for domain \"%s\" is in use.\n"), dom_name); + } else { + fprintf(stderr, _("Unable to lock tty %s: %s\n"), + tty, strerror(errno)); + } + exit(EXIT_FAILURE); + } + __priv_bracket(PRIV_OFF); + + free(tty);

When forward porting you can use VIR_FREE, from memory.h

+retry: + + ttyfd = open_tty(); + + if (!retrying && verbose) { + printf("Connected to domain %s\n", dom_name); + printf("Escape character is '^]'\n"); + } + + if (tcgetattr(STDIN_FILENO, &ttyattr) < 0) { + fprintf(stderr, _("Unable to get tty attributes: %s\n"), + strerror(errno)); + exit(EXIT_FAILURE); + } + + rawattr = ttyattr; + cfmakeraw(&rawattr); + + if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { + fprintf(stderr, _("Unable to set tty attributes: %s\n"), + strerror(errno)); + exit(EXIT_FAILURE); + } + + /* Now lets process STDIN & tty forever.... */ + for (; !got_signal ;) { + unsigned int i; + struct pollfd fds[] = { + { STDIN_FILENO, POLLIN, 0 }, + { ttyfd, POLLIN, 0 }, + }; + + /* Wait for data to be available for reading on + STDIN or the tty */ + if (poll(fds, (sizeof(fds)/sizeof(struct pollfd)), -1) < 0) { + if (got_signal) + goto cleanup; + + if (errno == EINTR || errno == EAGAIN) + continue; + + fprintf(stderr, _("Failure waiting for I/O: %s\n"), + strerror(errno)); + goto cleanup; + } + + for (i = 0 ; i < (sizeof(fds)/sizeof(struct pollfd)) ; i++) { + if (!fds[i].revents) + continue; + + /* Process incoming data available for read */ + if (fds[i].revents & POLLIN) { + char buf[4096]; + int got, sent = 0, destfd; + + if ((got = read(fds[i].fd, buf, sizeof(buf))) < 0) { + fprintf(stderr, _("Failure reading input: %s\n"), + strerror(errno)); + goto cleanup; + } + + if (!got) { + tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr); + if (!check_for_reboot()) + goto done; + close_tty(ttyfd); + retrying = 1; + goto retry; + } + + if (got == 1 && buf[0] == CTRL_CLOSE_BRACKET) + goto done; + + /* Data from stdin goes to the TTY, + data from the TTY goes to STDOUT */ + if (fds[i].fd == STDIN_FILENO) + destfd = ttyfd; + else + destfd = STDOUT_FILENO; + + while (sent < got) { + int done; + if ((done = write(destfd, buf + sent, got - sent)) <= 0) { + fprintf(stderr, _("Failure writing output: %s\n"), + strerror(errno)); + goto cleanup; + } + sent += done; + } + } else if (fds[i].revents & POLLHUP) { + tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr); + if (!check_for_reboot()) + goto done;

I like the support for re-connecting after reboot. At the same time I wonder if we need to make the an optional command line flag. Some apps using virsh console, may rely on the fact that it exits when a VM shuts down. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|