On Wed, Jan 14, 2009 at 07:21:10AM -0800, john.levon@xxxxxxx wrote:
> # HG changeset patch
> # User John Levon <john.levon@xxxxxxx>
> # 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@xxxxxxx>
>
> 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@xxxxxxxxxx>
> - */
> -
> -#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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list