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 :| -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list