[libvirt] [PATCH] Introduce virt-console
# HG changeset patch # User John Levon <john.levon@sun.com> # Date 1231946128 28800 # Node ID dd17b3062611925baa2698ff5923579d0f2cd34e # Parent a0d98d39955f4f304d318c7c780742ab929eb351 Introduce virt-console Separate console handling out into a separate binary to allow management of privileges. Signed-off-by: John Levon <john.levon@sun.com> diff --git a/src/Makefile.am b/src/Makefile.am --- a/src/Makefile.am +++ b/src/Makefile.am @@ -479,12 +479,16 @@ libvirt_test_la_LDFLAGS = $(test_LDFLAGS libvirt_test_la_LDFLAGS = $(test_LDFLAGS) libvirt_test_la_CFLAGS = $(COVERAGE_CFLAGS) +libexec_PROGRAMS = virt-console + +virt_console_SOURCES = console.c +virt_console_CFLAGS = $(COVERAGE_CFLAGS) +virt_console_LDFLAGS = $(WARN_CFLAGS) $(COVERAGE_LDFLAGS) +virt_console_LDADD = libvirt.la ../gnulib/lib/libgnu.la $(VIRSH_LIBS) + bin_PROGRAMS = virsh -virsh_SOURCES = \ - console.c console.h \ - virsh.c - +virsh_SOURCES = virsh.c virsh_LDFLAGS = $(WARN_CFLAGS) $(COVERAGE_LDFLAGS) virsh_LDADD = \ $(STATIC_BINARIES) \ @@ -554,8 +558,6 @@ virsh_win_icon.$(OBJEXT): virsh_win_icon --output-format coff --output $@ endif -libexec_PROGRAMS = - if WITH_STORAGE_DISK if WITH_LIBVIRTD libexec_PROGRAMS += libvirt_parthelper diff --git a/src/console.c b/src/console.c --- a/src/console.c +++ b/src/console.c @@ -1,7 +1,10 @@ /* - * console.c: A dumb serial console client + * virt-console: client for connecting to domain consoles. * * Copyright (C) 2007, 2008 Red Hat, Inc. + * + * Copyright 2008 Sun Microsystems, Inc. All rights reserved. + * Use is subject to license terms. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -22,34 +25,65 @@ #include <config.h> -#ifndef __MINGW32__ - #include <stdio.h> +#include <errno.h> +#include <stdlib.h> +#include <termios.h> +#include <signal.h> #include <sys/types.h> -#include <sys/stat.h> +#include <unistd.h> #include <fcntl.h> -#include <termios.h> #include <poll.h> -#include <string.h> -#include <errno.h> -#include <unistd.h> -#include <signal.h> - -#include "console.h" +#include <getopt.h> +#include <locale.h> + +#include <libxml/parser.h> +#include <libxml/tree.h> +#include <libxml/xpath.h> + #include "internal.h" +#include "memory.h" #include "util.h" +#include "xml.h" /* ie Ctrl-] as per telnet */ #define CTRL_CLOSE_BRACKET '\35' -static int got_signal = 0; -static void do_signal(int sig ATTRIBUTE_UNUSED) { +static int got_signal; +static int verbose; +static const char *dom_name; +static const char *conn_name; + +#ifdef __sun +#include <stropts.h> +#include <priv.h> + +#define PU_RESETGROUPS 0x0001 /* Remove supplemental groups */ +#define PU_LIMITPRIVS 0x0002 /* L=P */ +#define PU_INHERITPRIVS 0x0004 /* I=P */ +#define PU_CLEARLIMITSET 0x0008 /* L=0 */ + +extern int __init_suid_priv(int, ...); +extern int __priv_bracket(priv_op_t); + +#ifndef PRIV_XVM_CONTROL +#define PRIV_XVM_CONTROL ((const char *)"xvm_control") +#endif +#ifndef PRIV_VIRT_MANAGE +#define PRIV_VIRT_MANAGE ((const char *)"virt_manage") +#endif + +#endif /* __sun */ + +static void +do_signal(int sig ATTRIBUTE_UNUSED) +{ got_signal = 1; } #ifndef HAVE_CFMAKERAW static void -cfmakeraw (struct termios *attr) +cfmakeraw(struct termios *attr) { attr->c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR | IGNCR | ICRNL | IXON); @@ -57,46 +91,432 @@ cfmakeraw (struct termios *attr) 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 */ -int vshRunConsole(const char *tty) { - int ttyfd, ret = -1; - struct termios ttyattr, rawattr; +static void make_tty_raw(int fd, struct termios *attr) +{ + struct termios ttyattr; + struct termios rawattr; + + if (attr == NULL) + attr = &ttyattr; + + if (tcgetattr(fd, attr) < 0) { + fprintf(stderr, _("Unable to get tty attributes: %s\n"), + strerror(errno)); + exit(EXIT_FAILURE); + } + + rawattr = *attr; + cfmakeraw(&rawattr); + + if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { + fprintf(stderr, _("Unable to set tty attributes: %s\n"), + strerror(errno)); + exit(EXIT_FAILURE); + } +} + +#ifdef __sun + +/* + * Verify that the real user invoking this setuid-root executable has + * the needed privilege, then drop root and all the privileges we don't + * need before continuing. + */ +static void +setup_perms(void) +{ + if (seteuid(getuid()) == -1) { + perror("seteuid failed"); + exit(1); + } + + if (!priv_ineffect(PRIV_VIRT_MANAGE)) { + fprintf(stderr, "virt-console: permission denied\n"); + exit(1); + } + + if (seteuid(0) == -1) { + perror("seteuid failed"); + exit(1); + } + + /* + * We need to be able to talk to libvirt and open pty's. + */ + __init_suid_priv(PU_RESETGROUPS | PU_CLEARLIMITSET, + PRIV_VIRT_MANAGE, PRIV_FILE_DAC_READ, + PRIV_FILE_DAC_WRITE, NULL); +} + +#else +#define setup_perms() +#define __priv_bracket(a) +#endif + +static void +usage(int exitval) +{ + FILE *f = stderr; + + if (exitval == EXIT_SUCCESS) + f = stdout; + + fprintf(f, _("usage: virt-console [options] domain\n\n" + " options:\n" + " -c | --connect <uri> hypervisor connection URI\n" + " -h | --help this help\n" + " -v | --verbose be verbose\n\n")); + + exit(exitval); +} + +static void +parse_args(int argc, char *argv[]) +{ + int idx = 0; + int arg; + + struct option opt[] = { + { "connect", 1, 0, 'c' }, + { "help", 0, 0, 'h' }, + { "verbose", 0, 0, 'v' }, + { 0, 0, 0, 0 } + }; + + while ((arg = getopt_long(argc, argv, "c:hv", opt, &idx)) != -1) { + switch (arg) { + case 'c': + conn_name = optarg; + break; + case 'h': + usage(EXIT_SUCCESS); + break; + case 'v': + verbose = 1; + break; + default: + usage(EXIT_FAILURE); + break; + } + } + + if ((argc - optind) != 1) + usage(EXIT_FAILURE); + + dom_name = argv[optind]; + + if (conn_name == NULL) + conn_name = getenv("VIRSH_DEFAULT_CONNECT_URI"); +} + +static int +get_domain(virConnectPtr *conn, virDomainPtr *dom, + virDomainInfo *info, int lookup_by_id) +{ + int ret = 0; + int id; + + __priv_bracket(PRIV_ON); + + printf("1\n"); + *conn = virConnectOpenAuth(conn_name, virConnectAuthPtrDefault, + VIR_CONNECT_RO); + printf("2\n"); + 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 && + virStrToLong_i(dom_name, NULL, 10, &id) == 0 && id >= 0) + *dom = virDomainLookupByID(*conn, id); + + if (*dom == NULL) + goto out; + + if (info != NULL) { + if (virDomainGetInfo(*dom, info) < 0) + goto out; + } + + ret = 1; + +out: + if (ret == 0) { + if (*dom != NULL) + virDomainFree(*dom); + virConnectClose(*conn); + } + __priv_bracket(PRIV_OFF); + return ret; +} + +static void +put_domain(virConnectPtr conn, virDomainPtr dom) +{ + __priv_bracket(PRIV_ON); + if (dom != NULL) + virDomainFree(dom); + virConnectClose(conn); + __priv_bracket(PRIV_OFF); +} + +static char * +get_domain_tty(void) +{ + xmlXPathContextPtr ctxt = NULL; + xmlDocPtr xml = NULL; + virConnectPtr conn = NULL; + virDomainPtr dom = NULL; + char *doc = NULL; + char *tty = NULL; + + if (!get_domain(&conn, &dom, NULL, 1)) { + fprintf(stderr, _("Couldn't find domain \"%s\".\n"), dom_name); + exit(EXIT_FAILURE); + } + + __priv_bracket(PRIV_ON); + doc = virDomainGetXMLDesc(dom, 0); + __priv_bracket(PRIV_OFF); + + put_domain(conn, dom); + conn = NULL; + dom = NULL; + + if (doc == NULL) + goto cleanup; + + xml = xmlReadDoc((const xmlChar *) doc, "domain.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOWARNING); + if (xml == NULL) + goto cleanup; + + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) + goto cleanup; + + tty = virXPathString(conn, "string(/domain/devices/console/@tty)", ctxt); + +cleanup: + VIR_FREE(doc); + xmlXPathFreeContext(ctxt); + if (xml != NULL) + xmlFreeDoc(xml); + return tty; +} + +static int +domain_is_running(void) +{ + virConnectPtr conn = NULL; + virDomainPtr dom = NULL; + virDomainInfo info; + int ret = -1; + + if (!get_domain(&conn, &dom, &info, 1)) + return -1; + + switch (info.state) { + case VIR_DOMAIN_RUNNING: + case VIR_DOMAIN_BLOCKED: + case VIR_DOMAIN_PAUSED: + ret = 1; + break; + + case VIR_DOMAIN_NOSTATE: + case VIR_DOMAIN_CRASHED: + case VIR_DOMAIN_SHUTDOWN: + case VIR_DOMAIN_SHUTOFF: + ret = 0; + break; + + default: + break; + } + + put_domain(conn, dom); + return ret; +} + +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 < 6) { + sleep(1); + goto retry; + } + +out: + put_domain(conn, dom); + return ret; +} + +static int +open_tty(void) +{ + struct termios ttyattr; + 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) { + 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); + + VIR_FREE(tty); + +#ifdef __sun + /* + * The pty may come from either xend (with pygrub) or xenconsoled. + * It may have tty semantics set up, or not. While it isn't + * strictly necessary to have those semantics here, it is good to + * have a consistent state that is the same as under Linux. + * + * If tcgetattr fails, they have not been set up, so go ahead and + * set them up now, by pushing the ptem and ldterm streams modules. + */ + if (tcgetattr(ttyfd, &ttyattr) < 0) { + ioctl(ttyfd, I_PUSH, "ptem"); + ioctl(ttyfd, I_PUSH, "ldterm"); + tcgetattr(ttyfd, &ttyattr); + } + + cfmakeraw(&ttyattr); + tcsetattr(ttyfd, TCSANOW, &ttyattr); +#endif + + return ttyfd; +} + +static void +close_tty(int ttyfd) +{ + while (lockf(ttyfd, F_ULOCK, 0) && errno == EINTR) + ; + close(ttyfd); +} + +int +main(int argc, char *argv[]) +{ + struct termios ttyattr; + struct termios rawattr; void (*old_sigquit)(int); void (*old_sigterm)(int); void (*old_sigint)(int); void (*old_sighup)(int); void (*old_sigpipe)(int); - - - /* We do not want this to become the controlling TTY */ - if ((ttyfd = open(tty, O_NOCTTY | O_RDWR)) < 0) { - fprintf(stderr, _("unable to open tty %s: %s\n"), - tty, strerror(errno)); + int ret = EXIT_FAILURE; + int retrying = 0; + int ttyfd; + + setup_perms(); + + if (!setlocale(LC_ALL, "")) { + perror("setlocale"); return -1; } - /* Put STDIN into raw mode so that stuff typed - does not echo to the screen (the TTY reads will - result in it being echoed back already), and - also ensure Ctrl-C, etc is blocked, and misc - other bits */ - if (tcgetattr(STDIN_FILENO, &ttyattr) < 0) { - fprintf(stderr, _("unable to get tty attributes: %s\n"), - strerror(errno)); - goto closetty; - } - - rawattr = ttyattr; - cfmakeraw(&rawattr); - - if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { - fprintf(stderr, _("unable to set tty attributes: %s\n"), - strerror(errno)); - goto closetty; - } - + if (!bindtextdomain(GETTEXT_PACKAGE, LOCALEBASEDIR)) { + perror("bindtextdomain"); + return -1; + } + + if (!textdomain(GETTEXT_PACKAGE)) { + perror("textdomain"); + return -1; + } + + parse_args(argc, argv); /* Trap all common signals so that we can safely restore the original terminal settings on STDIN before the @@ -107,8 +527,36 @@ int vshRunConsole(const char *tty) { old_sigint = signal(SIGINT, do_signal); old_sighup = signal(SIGHUP, do_signal); old_sigpipe = signal(SIGPIPE, do_signal); - got_signal = 0; - + +retry: + + ttyfd = open_tty(); + + if (!retrying && verbose) { + printf("Connected to domain %s\n", dom_name); + printf("Escape character is '^]'\n"); + } + + /* + * Put STDIN into raw mode so that stuff typed does not echo to the + * screen (the TTY reads will result in it being echoed back + * already), and also ensure Ctrl-C, etc is blocked, and misc other + * bits + */ + 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 ;) { @@ -127,7 +575,7 @@ int vshRunConsole(const char *tty) { if (errno == EINTR || errno == EAGAIN) continue; - fprintf(stderr, _("failure waiting for I/O: %s\n"), + fprintf(stderr, _("Failure waiting for I/O: %s\n"), strerror(errno)); goto cleanup; } @@ -142,15 +590,21 @@ int vshRunConsole(const char *tty) { int got, sent = 0, destfd; if ((got = read(fds[i].fd, buf, sizeof(buf))) < 0) { - fprintf(stderr, _("failure reading input: %s\n"), + fprintf(stderr, _("Failure reading input: %s\n"), strerror(errno)); goto cleanup; } - /* Quit if end of file, or we got the Ctrl-] key */ - if (!got || - (got == 1 && - buf[0] == CTRL_CLOSE_BRACKET)) + 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, @@ -162,23 +616,31 @@ int vshRunConsole(const char *tty) { while (sent < got) { int done; - if ((done = safewrite(destfd, buf + sent, got - sent)) - <= 0) { - fprintf(stderr, _("failure writing output: %s\n"), + if ((done = safewrite(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; + close_tty(ttyfd); + retrying = 1; + goto retry; } else { /* Any other flag from poll is an error condition */ goto cleanup; } } } - done: - ret = 0; - - cleanup: + +done: + ret = EXIT_SUCCESS; +cleanup: + tcsetattr(STDIN_FILENO, TCSANOW, &ttyattr); + close_tty(ttyfd); /* Restore original signal handlers */ signal(SIGQUIT, old_sigpipe); @@ -187,14 +649,18 @@ int vshRunConsole(const char *tty) { signal(SIGQUIT, old_sigterm); signal(SIGQUIT, old_sigquit); - /* Put STDIN back into the (sane?) state we found - it in before starting */ - tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr); - - closetty: - close(ttyfd); - - return ret; -} - -#endif /* !__MINGW32__ */ + if (verbose) + printf("\nConnection to domain %s closed.", dom_name); + printf("\n"); + + exit(ret); +} + +/* + * Local variables: + * indent-tabs-mode: nil + * c-indent-level: 4 + * c-basic-offset: 4 + * tab-width: 4 + * End: + */ diff --git a/src/console.h b/src/console.h deleted file mode 100644 --- a/src/console.h +++ /dev/null @@ -1,32 +0,0 @@ -/* - * console.c: A dumb serial console client - * - * Copyright (C) 2007 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - * Daniel Berrange <berrange@redhat.com> - */ - -#ifndef __VIR_CONSOLE_H__ -#define __VIR_CONSOLE_H__ - -#ifndef __MINGW32__ - -int vshRunConsole(const char *tty); - -#endif /* !__MINGW32__ */ - -#endif /* __VIR_CONSOLE_H__ */ diff --git a/src/util.c b/src/util.c --- a/src/util.c +++ b/src/util.c @@ -240,7 +240,7 @@ __virExec(virConnectPtr conn, childout = *outfd; } #ifndef ENABLE_DEBUG - } else { + } else if (!(flags & VIR_EXEC_NO_PIPE)) { childout = null; #endif } @@ -271,7 +271,7 @@ __virExec(virConnectPtr conn, childerr = *errfd; } #ifndef ENABLE_DEBUG - } else { + } else if (!(flags & VIR_EXEC_NO_PIPE)) { childerr = null; #endif } @@ -652,6 +652,15 @@ virExec(virConnectPtr conn, return -1; } +int +virExecNoPipe(virConnectPtr conn, + char **argv ATTRIBUTE_UNUSED, + pid_t *retpid ATTRIBUTE_UNUSED) +{ + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); + return -1; +} + #endif /* __MINGW32__ */ /* Like gnulib's fread_file, but read no more than the specified maximum diff --git a/src/util.h b/src/util.h --- a/src/util.h +++ b/src/util.h @@ -36,6 +36,7 @@ enum { VIR_EXEC_NONE = 0, VIR_EXEC_NONBLOCK = (1 << 0), VIR_EXEC_DAEMON = (1 << 1), + VIR_EXEC_NO_PIPE = (1 << 2) }; int virExec(virConnectPtr conn, diff --git a/src/virsh.c b/src/virsh.c --- a/src/virsh.c +++ b/src/virsh.c @@ -29,6 +29,7 @@ #include <assert.h> #include <errno.h> #include <sys/stat.h> +#include <sys/wait.h> #include <inttypes.h> #include <libxml/parser.h> @@ -42,7 +43,6 @@ #include "internal.h" #include "buf.h" -#include "console.h" #include "util.h" static char *progname; @@ -469,6 +469,7 @@ static const vshCmdInfo info_console[] = static const vshCmdOptDef opts_console[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, + {"verbose", VSH_OT_BOOL, 0, gettext_noop("verbose console")}, {NULL, 0, 0, NULL} }; @@ -477,49 +478,39 @@ static int static int cmdConsole(vshControl *ctl, const vshCmd *cmd) { - xmlDocPtr xml = NULL; - xmlXPathObjectPtr obj = NULL; - xmlXPathContextPtr ctxt = NULL; - virDomainPtr dom; + const char *argv[] = { BINDIR "/virt-console", NULL, NULL, NULL, NULL }; + int verbose = vshCommandOptBool(cmd, "verbose"); int ret = FALSE; - char *doc; - - if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) - return FALSE; - - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - return FALSE; - - doc = virDomainGetXMLDesc(dom, 0); - if (!doc) - goto cleanup; - - xml = xmlReadDoc((const xmlChar *) doc, "domain.xml", NULL, - XML_PARSE_NOENT | XML_PARSE_NONET | - XML_PARSE_NOWARNING); - free(doc); - if (!xml) - goto cleanup; - ctxt = xmlXPathNewContext(xml); - if (!ctxt) - goto cleanup; - - 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, "%s", _("No console available for domain\n")); - } - xmlXPathFreeObject(obj); - - cleanup: - xmlXPathFreeContext(ctxt); - if (xml) - xmlFreeDoc(xml); - virDomainFree(dom); - return ret; + pid_t pid; + int argpos = 1; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + if (verbose) + argv[argpos++] = "--verbose"; + + if (ctl->name != NULL) { + argv[argpos++] = "--connect"; + argv[argpos++] = ctl->name; + } + + if (!(argv[argpos] = vshCommandOptString(cmd, "domain", NULL))) { + vshError(ctl, FALSE, _("Undefined domain name or id.")); + return FALSE; + } + + ret = virExec(ctl->conn, argv, NULL, 0, &pid, STDIN_FILENO, NULL, + NULL, VIR_EXEC_NO_PIPE); + + if (ret == -1) { + vshError(ctl, FALSE, _("Couldn't execute virt-console.")); + return FALSE; + } + + waitpid(pid, NULL, 0); + + return TRUE; } #else /* __MINGW32__ */ @@ -4583,6 +4574,7 @@ cmdTTYConsole(vshControl *ctl, const vsh goto cleanup; } vshPrint(ctl, "%s\n", (const char *)obj->stringval); + ret = TRUE; cleanup: xmlXPathFreeObject(obj);
On Wed, Jan 14, 2009 at 07:21:10AM -0800, john.levon@sun.com wrote:
+#ifdef __sun
I'm really unhappy about adding #ifdef __sun everywhere in this file. Configure should detect the features that are needed / available / missing, and the #ifdefs should go on the result of that. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones Read my OCaml programming blog: http://camltastic.blogspot.com/ Fedora now supports 68 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
On Thu, Jan 15, 2009 at 11:47:13AM +0000, Richard W.M. Jones wrote:
On Wed, Jan 14, 2009 at 07:21:10AM -0800, john.levon@sun.com wrote:
+#ifdef __sun
I'm really unhappy about adding #ifdef __sun everywhere in this file. Configure should detect the features that are needed / available / missing, and the #ifdefs should go on the result of that.
This is Solaris stuff that doesn't and won't exist anywhere else. It would just be moving the #ifdef __sun into configure.in - I can't think what advantage you would be expecting from this, or indeed how to even test the need to do the I_PUSHes. regards john
On Thu, Jan 15, 2009 at 12:59:36PM +0000, John Levon wrote:
On Thu, Jan 15, 2009 at 11:47:13AM +0000, Richard W.M. Jones wrote:
On Wed, Jan 14, 2009 at 07:21:10AM -0800, john.levon@sun.com wrote:
+#ifdef __sun
I'm really unhappy about adding #ifdef __sun everywhere in this file. Configure should detect the features that are needed / available / missing, and the #ifdefs should go on the result of that.
This is Solaris stuff that doesn't and won't exist anywhere else. It would just be moving the #ifdef __sun into configure.in - I can't think what advantage you would be expecting from this, [...]
The problem is that someone comes along needing, say, AIX support or Watcom on Windows or whatever, and before you know it you've got code like this ... http://svn.python.org/projects/python/branches/py3k/Modules/posixmodule.c We really should go by features detected in configure, not assumptions about the platform which may not even be true.
or indeed how to even test the need to do the I_PUSHes.
The autoconf manual is voluminous but comprehensive: http://www.gnu.org/software/autoconf/manual/autoconf.html For the I_PUSHes I think the best way is to have a tiny test program and use AC_COMPILE_IFELSE: http://www.gnu.org/software/autoconf/manual/autoconf.html#Running-the-Compil... (There's an example of how to use it in the preceeding section, 6.3, in the manual). If that doesn't help, we have Jim Meyering on this list who is the world's expert at this sort of thing. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones Read my OCaml programming blog: http://camltastic.blogspot.com/ Fedora now supports 68 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
On Thu, Jan 15, 2009 at 01:15:13PM +0000, Richard W.M. Jones wrote:
We really should go by features detected in configure, not assumptions about the platform which may not even be true.
That's my point. They are true, and they always will be. In fact, it's MUCH safer in these cases to check __sun. I can't check for stropts.h, since someone might have installed that icky STREAMS stuff on Linux, which would then break. I can't check for priv.h, since it's generic enough to exist in different form on other platforms. Even if least priv in the exact Solaris form gets ported to say BSD, it won't have __init_suid_priv(), for example. I'm not arguing against configure checks in the general case (e.g. cfmakeraw), but these two cases (STREAMS terminals and least priv) are very different. If I thought there were any possibility of this ever applying to another platform (like, say, DTrace), I'd be all for it.
or indeed how to even test the need to do the I_PUSHes.
The autoconf manual is voluminous but comprehensive:
I'm not asking how to write autoconf macros. I'm asking how to write a build-time test that ptys are STREAMS-based (and possibly just Solaris STREAMS), without it being essentially #ifdef __sun. Of course, my preferred fix would be for Solaris to drop STREAMS, but that's a vain hope :) regards, john
On Thu, Jan 15, 2009 at 01:27:00PM +0000, John Levon wrote:
On Thu, Jan 15, 2009 at 01:15:13PM +0000, Richard W.M. Jones wrote:
We really should go by features detected in configure, not assumptions about the platform which may not even be true.
That's my point. They are true, and they always will be. In fact, it's MUCH safer in these cases to check __sun. I can't check for stropts.h, since someone might have installed that icky STREAMS stuff on Linux, which would then break. I can't check for priv.h, since it's generic enough to exist in different form on other platforms. Even if least priv in the exact Solaris form gets ported to say BSD, it won't have __init_suid_priv(), for example.
I'm not arguing against configure checks in the general case (e.g. cfmakeraw), but these two cases (STREAMS terminals and least priv) are very different.
I think configure.ac checks for things like the ioctl flags for TTY attr settings are reasonable, because it is conceivable that other OS may implement such flags later. For stuff like the STREAMS/privilege bits they are so completely Solaris specific its easier to just #ifdef __sun them 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 :|
On Thu, Jan 15, 2009 at 01:37:01PM +0000, Daniel P. Berrange wrote:
On Thu, Jan 15, 2009 at 01:27:00PM +0000, John Levon wrote:
On Thu, Jan 15, 2009 at 01:15:13PM +0000, Richard W.M. Jones wrote:
We really should go by features detected in configure, not assumptions about the platform which may not even be true.
That's my point. They are true, and they always will be. In fact, it's MUCH safer in these cases to check __sun. I can't check for stropts.h, since someone might have installed that icky STREAMS stuff on Linux, which would then break. I can't check for priv.h, since it's generic enough to exist in different form on other platforms. Even if least priv in the exact Solaris form gets ported to say BSD, it won't have __init_suid_priv(), for example.
I'm not arguing against configure checks in the general case (e.g. cfmakeraw), but these two cases (STREAMS terminals and least priv) are very different.
I think configure.ac checks for things like the ioctl flags for TTY attr settings are reasonable, because it is conceivable that other OS may implement such flags later. For stuff like the STREAMS/privilege bits they are so completely Solaris specific its easier to just #ifdef __sun them
One caveat on the latter though - if there's solaris specific stuff that is only available on newer Solaris versions, then a configure.ac check would be sensible to allow it to still build on earlier versions, depending on how much back-compat support you want todo, and whether its still practical to work without the feature in question. 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 :|
On Thu, Jan 15, 2009 at 01:40:06PM +0000, Daniel P. Berrange wrote:
One caveat on the latter though - if there's solaris specific stuff that is only available on newer Solaris versions, then a configure.ac check would be sensible to allow it to still build on earlier versions, depending on how much back-compat support you want todo, and whether its still practical to work without the feature in question.
Earlier versions would be Solaris 10. Whilst it's conceivable that at some point we would have libvirt there, it's rather unlikely, and in such a case would involve a lot more work. regards john
On Thu, Jan 15, 2009 at 01:27:00PM +0000, John Levon wrote:
On Thu, Jan 15, 2009 at 01:15:13PM +0000, Richard W.M. Jones wrote:
We really should go by features detected in configure, not assumptions about the platform which may not even be true.
That's my point. They are true, and they always will be. In fact, it's MUCH safer in these cases to check __sun. I can't check for stropts.h, since someone might have installed that icky STREAMS stuff on Linux, which would then break.
Doesn't working STREAMS exist on UnixWare too? I still think the right thing to do is to detect STREAMS. If someone installs broken STREAMS on Linux, then it is their responsibility to find a more discrete test which differentiates between broken STREAMS and working STREAMS on Solaris/UnixWare.
I can't check for priv.h, since it's generic enough to exist in different form on other platforms. Even if least priv in the exact Solaris form gets ported to say BSD, it won't have __init_suid_priv(), for example.
Right, but don't just check for the header. Check for both the header and some common symbol inside it.
Of course, my preferred fix would be for Solaris to drop STREAMS, but that's a vain hope :)
There's the problem - if Solaris drop streams, the #ifdef __sun checks will be incorrect. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On Thu, Jan 15, 2009 at 01:40:03PM +0000, Richard W.M. Jones wrote:
Doesn't working STREAMS exist on UnixWare too? I still think the
UnixWare? Seriously?
right thing to do is to detect STREAMS. If someone installs broken STREAMS on Linux, then it is their responsibility to find a more
They're not broken - installing the STREAMS support doesn't magically rewrite the Linux kernel pty code to use it. You seem to be arguing that I should introduce potential bugs, with zero benefit.
Of course, my preferred fix would be for Solaris to drop STREAMS, but that's a vain hope :)
There's the problem - if Solaris drop streams, the #ifdef __sun checks will be incorrect.
No - if this ever happens, it will not break pty-using code. Again, what test are you thinking of to detect that the pty code is STREAMS based? I'm not being obstructive here - I genuinely cannot conceive what such a test would look like. regards, john
On Thu, Jan 15, 2009 at 01:51:21PM +0000, John Levon wrote:
No - if this ever happens, it will not break pty-using code. Again, what test are you thinking of to detect that the pty code is STREAMS based? I'm not being obstructive here - I genuinely cannot conceive what such a test would look like.
I think something as simple as: AC_CHECK_FUNCS([isastream]) is sufficient to check whether the libc claims to support STREAMS. If it turns out that someone has installed a buggy STREAMS implementation, we can make the check more fine-grained later. Rich. http://www.opengroup.org/onlinepubs/009695399/functions/isastream.html -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
On Thu, Jan 15, 2009 at 02:05:14PM +0000, Richard W.M. Jones wrote:
No - if this ever happens, it will not break pty-using code. Again, what test are you thinking of to detect that the pty code is STREAMS based? I'm not being obstructive here - I genuinely cannot conceive what such a test would look like.
I think something as simple as:
AC_CHECK_FUNCS([isastream])
is sufficient to check whether the libc claims to support STREAMS.
That wasn't what I asked for.
If it turns out that someone has installed a buggy STREAMS implementation, we can make the check more fine-grained later.
Again, it's NOT buggy for isastream to exist, and pty's to be non-STREAMS. My way: - definitely not buggy, less code Your way: - possibly buggy, more code What platform are you expecting this extra code to help? It's not the BSDs, Mac OS X, Solaris, Linux, or Windows. I'll buy you tea at the Ritz if someone ports libvirt to UnixWare :) regards, john
On Wed, Jan 14, 2009 at 07:21:10AM -0800, john.levon@sun.com wrote:
# HG changeset patch # User John Levon <john.levon@sun.com> # Date 1231946128 28800 # Node ID dd17b3062611925baa2698ff5923579d0f2cd34e # Parent a0d98d39955f4f304d318c7c780742ab929eb351 Introduce virt-console
Separate console handling out into a separate binary to allow management of privileges.
Signed-off-by: John Levon <john.levon@sun.com>
diff --git a/src/Makefile.am b/src/Makefile.am --- a/src/Makefile.am +++ b/src/Makefile.am @@ -479,12 +479,16 @@ libvirt_test_la_LDFLAGS = $(test_LDFLAGS libvirt_test_la_LDFLAGS = $(test_LDFLAGS) libvirt_test_la_CFLAGS = $(COVERAGE_CFLAGS)
+libexec_PROGRAMS = virt-console
This can just be bin_PROGRAMS - not need to hide it outside of /usr/bin - its fine to let users just run virt-console directly if they wish
#ifndef HAVE_CFMAKERAW static void -cfmakeraw (struct termios *attr) +cfmakeraw(struct termios *attr) { attr->c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR | IGNCR | ICRNL | IXON); @@ -57,46 +91,432 @@ cfmakeraw (struct termios *attr) 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
Both those constants are available on Linux too, so seems reasonable to just remove the #ifdef here.
+static void make_tty_raw(int fd, struct termios *attr) +{ + struct termios ttyattr; + struct termios rawattr; + + if (attr == NULL) + attr = &ttyattr; + + if (tcgetattr(fd, attr) < 0) { + fprintf(stderr, _("Unable to get tty attributes: %s\n"), + strerror(errno)); + exit(EXIT_FAILURE); + } + + rawattr = *attr; + cfmakeraw(&rawattr); + + if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { + fprintf(stderr, _("Unable to set tty attributes: %s\n"), + strerror(errno)); + exit(EXIT_FAILURE); + } +}
This make_tty_raw function does not appear to be used anywhere in the patch.
+ +static void +usage(int exitval) +{ + FILE *f = stderr; + + if (exitval == EXIT_SUCCESS) + f = stdout; + + fprintf(f, _("usage: virt-console [options] domain\n\n" + " options:\n" + " -c | --connect <uri> hypervisor connection URI\n" + " -h | --help this help\n" + " -v | --verbose be verbose\n\n")); + + exit(exitval); +}
We need to add an explicit argument to turn on the automatic reconnect of VMs when they reboot. Existing apps calling virsh console rely on its current semantics which are to exit upon domain reboot and we can't break them I'd suggest that when tracking across reboots, we should wait forever by default, and hav an optional timeout arg if you want to make it only wait 10 seconds. -r | --reconnect reconnect when a VM reboots -t | --timeout=N exit if VM hasn't started after N seconds Finally, if we're waiting after reboots, it seems sensible to also have the ability for wait for initial startup -w | --wait wait for VM to start if not running That lets people launch virt-console before they start the VM for the first time, so they can catch initial mesages easily.
+static int +get_domain(virConnectPtr *conn, virDomainPtr *dom, + virDomainInfo *info, int lookup_by_id) +{ + int ret = 0; + int id; + + __priv_bracket(PRIV_ON); + + printf("1\n"); + *conn = virConnectOpenAuth(conn_name, virConnectAuthPtrDefault, + VIR_CONNECT_RO); + printf("2\n");
Debug prints :-)
+ 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 && + virStrToLong_i(dom_name, NULL, 10, &id) == 0 && id >= 0) + *dom = virDomainLookupByID(*conn, id); + + if (*dom == NULL) + goto out; + + if (info != NULL) { + if (virDomainGetInfo(*dom, info) < 0) + goto out; + } + + ret = 1; + +out: + if (ret == 0) { + if (*dom != NULL) + virDomainFree(*dom); + virConnectClose(*conn); + } + __priv_bracket(PRIV_OFF); + return ret; +} + +static void +put_domain(virConnectPtr conn, virDomainPtr dom) +{ + __priv_bracket(PRIV_ON); + if (dom != NULL) + virDomainFree(dom); + virConnectClose(conn); + __priv_bracket(PRIV_OFF); +}
+static char * +get_domain_tty(void) +{ + xmlXPathContextPtr ctxt = NULL; + xmlDocPtr xml = NULL; + virConnectPtr conn = NULL; + virDomainPtr dom = NULL; + char *doc = NULL; + char *tty = NULL; + + if (!get_domain(&conn, &dom, NULL, 1)) { + fprintf(stderr, _("Couldn't find domain \"%s\".\n"), dom_name); + exit(EXIT_FAILURE); + }
If addin support for a --wait flag, we shouldn't treat this as fatal - we should simply wait arond for it to come into existance
+static int +open_tty(void) +{ + struct termios ttyattr; + 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) { + 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); + + VIR_FREE(tty); + +#ifdef __sun + /* + * The pty may come from either xend (with pygrub) or xenconsoled. + * It may have tty semantics set up, or not. While it isn't + * strictly necessary to have those semantics here, it is good to + * have a consistent state that is the same as under Linux. + * + * If tcgetattr fails, they have not been set up, so go ahead and + * set them up now, by pushing the ptem and ldterm streams modules. + */ + if (tcgetattr(ttyfd, &ttyattr) < 0) { + ioctl(ttyfd, I_PUSH, "ptem"); + ioctl(ttyfd, I_PUSH, "ldterm"); + tcgetattr(ttyfd, &ttyattr); + } + + cfmakeraw(&ttyattr); + tcsetattr(ttyfd, TCSANOW, &ttyattr); +#endif
The caller of open_tty() is also doing the getattr/makeraw/setattr operation, so this block appears to be redundant - just need to move the 2 ioctl() calls into the caller too. NB, this also needs to be in the caller, so they can keep a record of the original terminal attributes to do correct restore of terminal state on exit of virt-console.
-#endif /* !__MINGW32__ */ + if (verbose) + printf("\nConnection to domain %s closed.", dom_name); + printf("\n"); + + exit(ret); +} + +/* + * Local variables: + * indent-tabs-mode: nil + * c-indent-level: 4 + * c-basic-offset: 4 + * tab-width: 4 + * End: + */ diff --git a/src/console.h b/src/console.h deleted file mode 100644 --- a/src/console.h +++ /dev/null @@ -1,32 +0,0 @@ -/* - * console.c: A dumb serial console client - * - * Copyright (C) 2007 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - * Daniel Berrange <berrange@redhat.com> - */ - -#ifndef __VIR_CONSOLE_H__ -#define __VIR_CONSOLE_H__ - -#ifndef __MINGW32__ - -int vshRunConsole(const char *tty); - -#endif /* !__MINGW32__ */ - -#endif /* __VIR_CONSOLE_H__ */ diff --git a/src/util.c b/src/util.c --- a/src/util.c +++ b/src/util.c @@ -652,6 +652,15 @@ virExec(virConnectPtr conn, return -1; }
+int +virExecNoPipe(virConnectPtr conn, + char **argv ATTRIBUTE_UNUSED, + pid_t *retpid ATTRIBUTE_UNUSED) +{ + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); + return -1; +}
This doesn't appear to be used now we have a flag for NO_PIPE on the reguler virExec call.
+ if (ctl->name != NULL) { + argv[argpos++] = "--connect"; + argv[argpos++] = ctl->name; + }
Hmm, I'm not sure that ctl->name is always guarenteed to be non-null. For safety we should call virConnectGetURI(conn) which gets the actual final URI that libvirt opened, rather than the one virsh got. 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 :|
On Thu, Jan 15, 2009 at 01:28:37PM +0000, Daniel P. Berrange wrote:
+libexec_PROGRAMS = virt-console
This can just be bin_PROGRAMS - not need to hide it outside of /usr/bin - its fine to let users just run virt-console directly if they wish
Solaris policy is not to introduce plumbing into the user's PATH. virt-console is undocumented and there is no advantage to running it directly. If it were in PATH we would have to document it, and we have no intention of doing that...
We need to add an explicit argument to turn on the automatic reconnect of VMs when they reboot. Existing apps calling virsh console rely on its current semantics which are to exit upon domain reboot and we can't break them
We argued about this last time. Looks like we'll have to keep this change private, and let Linux users suffer. Oh well :)
I'd suggest that when tracking across reboots, we should wait forever by default, and hav an optional timeout arg if you want to make it only wait 10 seconds.
-r | --reconnect reconnect when a VM reboots -t | --timeout=N exit if VM hasn't started after N seconds
Finally, if we're waiting after reboots, it seems sensible to also have the ability for wait for initial startup
-w | --wait wait for VM to start if not running
That lets people launch virt-console before they start the VM for the first time, so they can catch initial mesages easily.
Sure.
+ if (tcgetattr(ttyfd, &ttyattr) < 0) { + ioctl(ttyfd, I_PUSH, "ptem"); + ioctl(ttyfd, I_PUSH, "ldterm"); + tcgetattr(ttyfd, &ttyattr); + } + + cfmakeraw(&ttyattr); + tcsetattr(ttyfd, TCSANOW, &ttyattr); +#endif
The caller of open_tty() is also doing the getattr/makeraw/setattr operation, so this block appears to be redundant - just need to
Nope, that's on STDIN, not the pty slave. Stupid STREAMS semantics. regards john
On Thu, Jan 15, 2009 at 01:45:50PM +0000, John Levon wrote:
On Thu, Jan 15, 2009 at 01:28:37PM +0000, Daniel P. Berrange wrote:
+libexec_PROGRAMS = virt-console
This can just be bin_PROGRAMS - not need to hide it outside of /usr/bin - its fine to let users just run virt-console directly if they wish
Solaris policy is not to introduce plumbing into the user's PATH. virt-console is undocumented and there is no advantage to running it directly. If it were in PATH we would have to document it, and we have no intention of doing that...
This is the same as the Fedora policy too. If users aren't meant to use virt-console directly then it should be in libexec as you say. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
On Thu, Jan 15, 2009 at 01:45:50PM +0000, John Levon wrote:
On Thu, Jan 15, 2009 at 01:28:37PM +0000, Daniel P. Berrange wrote:
+libexec_PROGRAMS = virt-console
This can just be bin_PROGRAMS - not need to hide it outside of /usr/bin - its fine to let users just run virt-console directly if they wish
Solaris policy is not to introduce plumbing into the user's PATH. virt-console is undocumented and there is no advantage to running it directly. If it were in PATH we would have to document it, and we have no intention of doing that...
I'll volunteer to write a manual page for virt-console, since even existing manpage for 'virsh console' is non-existant.
We need to add an explicit argument to turn on the automatic reconnect of VMs when they reboot. Existing apps calling virsh console rely on its current semantics which are to exit upon domain reboot and we can't break them
We argued about this last time. Looks like we'll have to keep this change private, and let Linux users suffer. Oh well :)
You explicitly break virt-install by doing this. Have virt-console provide the more sensible default auto-reconnect semantics, and make 'virsh console' call it with a flag to turn this off to preserve existing semantics & not break users like virt-install.
+ if (tcgetattr(ttyfd, &ttyattr) < 0) { + ioctl(ttyfd, I_PUSH, "ptem"); + ioctl(ttyfd, I_PUSH, "ldterm"); + tcgetattr(ttyfd, &ttyattr); + } + + cfmakeraw(&ttyattr); + tcsetattr(ttyfd, TCSANOW, &ttyattr); +#endif
The caller of open_tty() is also doing the getattr/makeraw/setattr operation, so this block appears to be redundant - just need to
Nope, that's on STDIN, not the pty slave. Stupid STREAMS semantics.
Oh, can you add a comment to this effect -- easy to miss that distinction when browsing the code :-) 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 :|
On Thu, Jan 15, 2009 at 01:52:08PM +0000, Daniel P. Berrange wrote:
Solaris policy is not to introduce plumbing into the user's PATH. virt-console is undocumented and there is no advantage to running it directly. If it were in PATH we would have to document it, and we have no intention of doing that...
I'll volunteer to write a manual page for virt-console, since even existing manpage for 'virsh console' is non-existant.
Hmm, I thought we'd picked up our virsh man page from upstream. But, I don't see it there.
We need to add an explicit argument to turn on the automatic reconnect of VMs when they reboot. Existing apps calling virsh console rely on its current semantics which are to exit upon domain reboot and we can't break them
We argued about this last time. Looks like we'll have to keep this change private, and let Linux users suffer. Oh well :)
You explicitly break virt-install by doing this.
Break how?
Have virt-console provide the more sensible default auto-reconnect semantics, and make 'virsh console' call it with a flag to turn this off to preserve existing semantics & not break users like virt-install.
This is horrible IMHO - the user has to run some strange command instead of virsh like they use for everything else? I'd rather not auto-reconnect than this.
The caller of open_tty() is also doing the getattr/makeraw/setattr operation, so this block appears to be redundant - just need to
Nope, that's on STDIN, not the pty slave. Stupid STREAMS semantics.
Oh, can you add a comment to this effect -- easy to miss that distinction when browsing the code :-)
Yep. regards, john
On Thu, Jan 15, 2009 at 01:56:56PM +0000, John Levon wrote:
On Thu, Jan 15, 2009 at 01:52:08PM +0000, Daniel P. Berrange wrote:
Solaris policy is not to introduce plumbing into the user's PATH. virt-console is undocumented and there is no advantage to running it directly. If it were in PATH we would have to document it, and we have no intention of doing that...
I'll volunteer to write a manual page for virt-console, since even existing manpage for 'virsh console' is non-existant.
Hmm, I thought we'd picked up our virsh man page from upstream. But, I don't see it there.
We need to add an explicit argument to turn on the automatic reconnect of VMs when they reboot. Existing apps calling virsh console rely on its current semantics which are to exit upon domain reboot and we can't break them
We argued about this last time. Looks like we'll have to keep this change private, and let Linux users suffer. Oh well :)
You explicitly break virt-install by doing this.
Break how?
It relies on virsh console exiting when the domain shuts down.
Have virt-console provide the more sensible default auto-reconnect semantics, and make 'virsh console' call it with a flag to turn this off to preserve existing semantics & not break users like virt-install.
This is horrible IMHO - the user has to run some strange command instead of virsh like they use for everything else? I'd rather not auto-reconnect than this.
Its not so strange in the context of all the other virt commands we have, in particular in relation to virt-viewer virt-viewer - graphical console virt-console - text console virt-top virt-df virt-manager virt-install ...etc.. If anything, virsh is the odd one out. 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 :|
On Thu, Jan 15, 2009 at 02:04:56PM +0000, Daniel P. Berrange wrote:
You explicitly break virt-install by doing this.
Break how?
It relies on virsh console exiting when the domain shuts down.
I'm confused, we've been using virt-install without apparent problems for quite some time like this?
Have virt-console provide the more sensible default auto-reconnect semantics, and make 'virsh console' call it with a flag to turn this off to preserve existing semantics & not break users like virt-install.
This is horrible IMHO - the user has to run some strange command instead of virsh like they use for everything else? I'd rather not auto-reconnect than this.
Its not so strange in the context of all the other virt commands we have, in particular in relation to virt-viewer
virt-viewer - graphical console
The existence of the other tools doesn't make it sensible for obscure behaviour changes between two ways of doing the exact same thing. The /only/ reason I'm introducing virt-console is to correctly implement the least priv support. virt-manager aside (which is more or less a graphical virsh/virt-install), all of these should have indeed been virsh commands. It's horrible that I have to use a separate tool with a different interface to install my guest, then virsh to manage it. regards, john
participants (5)
-
Daniel P. Berrange -
John Levon -
John Levon -
john.levon＠sun.com
-
Richard W.M. Jones