[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] RFC: virt-console



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]