|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] Fix xenconsole's "Could not read tty from store"
On Wed, Dec 19, 2007 at 12:00:25PM +0000, Samuel Thibault wrote:
> > > To my understanding, from the server side tcsetattr should only be
> > > performed on the master side (with effect on the slave side too).
> >
> > Here's another try, what does this do on Linux?
>
> That one seems to work fine.
Excellent. Keir, can you apply?
cheers
john
# HG changeset patch
# User john.levon@xxxxxxx
# Date 1198003657 28800
# Node ID 6f03f4ec458d4780314c015da214795b7b1cf195
# Parent 539cbabd97b5ff3d335de151636040bb2f4cd629
Fix master/slave handling in xenconsoled and qemu
Fix a number of problems with the pty handling:
- make openpty() implementation work on Solaris
- set raw on the slave fd, not the master, as the master doesn't
have a line discipline pushed on Solaris
- make sure we don't leak the slave fd returned from openpty()
- don't use the 'name' argument of openpty() as it's a security risk
- note behaviour of a zero read of the master on Solaris
- remove pointless tcget/setattr
Signed-off-by: John Levon <john.levon@xxxxxxx>
Signed-off-by: Samuel Thibault <samuel.thibault@xxxxxxxxxx>
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -36,10 +36,14 @@
#include <stdarg.h>
#include <sys/mman.h>
#include <sys/time.h>
+#include <assert.h>
#if defined(__NetBSD__) || defined(__OpenBSD__)
#include <util.h>
#elif defined(__linux__) || defined(__Linux__)
#include <pty.h>
+#endif
+#if defined(__sun__)
+#include <stropts.h>
#endif
#define MAX(a, b) (((a) > (b)) ? (a) : (b))
@@ -75,7 +79,8 @@ struct domain
struct domain
{
int domid;
- int tty_fd;
+ int master_fd;
+ int slave_fd;
int log_fd;
bool is_dead;
struct buffer buffer;
@@ -227,77 +232,90 @@ static int create_domain_log(struct doma
return fd;
}
+static void domain_close_tty(struct domain *dom)
+{
+ if (dom->master_fd != -1) {
+ close(dom->master_fd);
+ dom->master_fd = -1;
+ }
+
+ if (dom->slave_fd != -1) {
+ close(dom->slave_fd);
+ dom->slave_fd = -1;
+ }
+}
+
#ifdef __sun__
/* Once Solaris has openpty(), this is going to be removed. */
-int openpty(int *amaster, int *aslave, char *name,
- struct termios *termp, struct winsize *winp)
+static int openpty(int *amaster, int *aslave, char *name,
+ struct termios *termp, struct winsize *winp)
{
- int mfd, sfd;
+ const char *slave;
+ int mfd = -1, sfd = -1;
*amaster = *aslave = -1;
- mfd = sfd = -1;
mfd = open("/dev/ptmx", O_RDWR | O_NOCTTY);
if (mfd < 0)
- goto err0;
+ goto err;
if (grantpt(mfd) == -1 || unlockpt(mfd) == -1)
- goto err1;
+ goto err;
- /* This does not match openpty specification,
- * but as long as this does not hurt, this is acceptable.
- */
- mfd = sfd;
+ if ((slave = ptsname(mfd)) == NULL)
+ goto err;
- if (termp != NULL && tcgetattr(sfd, termp) < 0)
- goto err1;
+ if ((sfd = open(slave, O_RDONLY | O_NOCTTY)) == -1)
+ goto err;
+
+ if (ioctl(sfd, I_PUSH, "ptem") == -1)
+ goto err;
if (amaster)
*amaster = mfd;
if (aslave)
*aslave = sfd;
- if (name)
- strlcpy(name, ptsname(mfd), sizeof(slave));
if (winp)
ioctl(sfd, TIOCSWINSZ, winp);
+ assert(name == NULL);
+ assert(termp == NULL);
+
return 0;
-err1:
+err:
+ if (sfd != -1)
+ close(sfd);
close(mfd);
-err0:
return -1;
}
#endif
-
static int domain_create_tty(struct domain *dom)
{
- char slave[80];
- struct termios term;
+ const char *slave;
char *path;
- int master, slavefd;
int err;
bool success;
char *data;
unsigned int len;
- if (openpty(&master, &slavefd, slave, &term, NULL) < 0) {
- master = -1;
+ assert(dom->slave_fd == -1);
+ assert(dom->master_fd == -1);
+
+ if (openpty(&dom->master_fd, &dom->slave_fd, NULL, NULL, NULL) < 0) {
err = errno;
dolog(LOG_ERR, "Failed to create tty for domain-%d (errno = %i,
%s)",
dom->domid, err, strerror(err));
- return master;
+ return 0;
}
- cfmakeraw(&term);
- if (tcsetattr(master, TCSAFLUSH, &term) < 0) {
+ if ((slave = ptsname(dom->master_fd)) == NULL) {
err = errno;
- dolog(LOG_ERR, "Failed to set tty attribute for domain-%d
(errno = %i, %s)",
+ dolog(LOG_ERR, "Failed to get slave name for domain-%d (errno =
%i, %s)",
dom->domid, err, strerror(err));
goto out;
}
-
if (dom->use_consolepath) {
success = asprintf(&path, "%s/limit", dom->conspath) !=
@@ -340,15 +358,15 @@ static int domain_create_tty(struct doma
goto out;
}
- if (fcntl(master, F_SETFL, O_NONBLOCK) == -1)
+ if (fcntl(dom->master_fd, F_SETFL, O_NONBLOCK) == -1)
goto out;
- return master;
- out:
- close(master);
- return -1;
+ return 1;
+out:
+ domain_close_tty(dom);
+ return 0;
}
-
+
/* Takes tuples of names, scanf-style args, and void **, NULL terminated. */
int xs_gather(struct xs_handle *xs, const char *dir, ...)
{
@@ -454,10 +472,8 @@ static int domain_create_ring(struct dom
dom->local_port = rc;
dom->remote_port = remote_port;
- if (dom->tty_fd == -1) {
- dom->tty_fd = domain_create_tty(dom);
-
- if (dom->tty_fd == -1) {
+ if (dom->master_fd == -1) {
+ if (!domain_create_tty(dom)) {
err = errno;
xc_evtchn_close(dom->xce_handle);
dom->xce_handle = -1;
@@ -535,7 +551,8 @@ static struct domain *create_domain(int
dom->conspath = s;
strcat(dom->conspath, "/console");
- dom->tty_fd = -1;
+ dom->master_fd = -1;
+ dom->slave_fd = -1;
dom->log_fd = -1;
dom->is_dead = false;
@@ -597,14 +614,7 @@ static void remove_domain(struct domain
static void cleanup_domain(struct domain *d)
{
- if (d->tty_fd != -1) {
- close(d->tty_fd);
- d->tty_fd = -1;
- }
- if (d->log_fd != -1) {
- close(d->log_fd);
- d->log_fd = -1;
- }
+ domain_close_tty(d);
free(d->buffer.data);
d->buffer.data = NULL;
@@ -683,13 +693,17 @@ static void handle_tty_read(struct domai
if (len > sizeof(msg))
len = sizeof(msg);
- len = read(dom->tty_fd, msg, len);
- if (len < 1) {
- close(dom->tty_fd);
- dom->tty_fd = -1;
+ len = read(dom->master_fd, msg, len);
+ /*
+ * Note: on Solaris, len == 0 means the slave closed, and this
+ * is no problem, but Linux can't handle this usefully, so we
+ * keep the slave open for the duration.
+ */
+ if (len < 0) {
+ domain_close_tty(dom);
if (domain_is_valid(dom->domid)) {
- dom->tty_fd = domain_create_tty(dom);
+ domain_create_tty(dom);
} else {
shutdown_domain(dom);
}
@@ -703,8 +717,7 @@ static void handle_tty_read(struct domai
intf->in_prod = prod;
xc_evtchn_notify(dom->xce_handle, dom->local_port);
} else {
- close(dom->tty_fd);
- dom->tty_fd = -1;
+ domain_close_tty(dom);
shutdown_domain(dom);
}
}
@@ -716,17 +729,16 @@ static void handle_tty_write(struct doma
if (dom->is_dead)
return;
- len = write(dom->tty_fd, dom->buffer.data + dom->buffer.consumed,
+ len = write(dom->master_fd, dom->buffer.data + dom->buffer.consumed,
dom->buffer.size - dom->buffer.consumed);
if (len < 1) {
dolog(LOG_DEBUG, "Write failed on domain %d: %zd, %d\n",
dom->domid, len, errno);
- close(dom->tty_fd);
- dom->tty_fd = -1;
+ domain_close_tty(dom);
if (domain_is_valid(dom->domid)) {
- dom->tty_fd = domain_create_tty(dom);
+ domain_create_tty(dom);
} else {
shutdown_domain(dom);
}
@@ -895,13 +907,13 @@ void handle_io(void)
max_fd = MAX(evtchn_fd, max_fd);
}
- if (d->tty_fd != -1) {
+ if (d->master_fd != -1) {
if (!d->is_dead && ring_free_bytes(d))
- FD_SET(d->tty_fd, &readfds);
+ FD_SET(d->master_fd, &readfds);
if (!buffer_empty(&d->buffer))
- FD_SET(d->tty_fd, &writefds);
- max_fd = MAX(d->tty_fd, max_fd);
+ FD_SET(d->master_fd, &writefds);
+ max_fd = MAX(d->master_fd, max_fd);
}
}
@@ -951,10 +963,10 @@ void handle_io(void)
handle_ring_read(d);
}
- if (d->tty_fd != -1 && FD_ISSET(d->tty_fd, &readfds))
+ if (d->master_fd != -1 && FD_ISSET(d->master_fd,
&readfds))
handle_tty_read(d);
- if (d->tty_fd != -1 && FD_ISSET(d->tty_fd, &writefds))
+ if (d->master_fd != -1 && FD_ISSET(d->master_fd,
&writefds))
handle_tty_write(d);
if (d->is_dead)
diff --git a/tools/ioemu/vl.c b/tools/ioemu/vl.c
--- a/tools/ioemu/vl.c
+++ b/tools/ioemu/vl.c
@@ -65,6 +65,9 @@
#include <linux/rtc.h>
#include <linux/ppdev.h>
#endif
+#endif
+#if defined(__sun__)
+#include <stropts.h>
#endif
#endif
@@ -1801,7 +1804,65 @@ static int store_dev_info(char *devName,
return 0;
}
-#if defined(__linux__) || defined(__NetBSD__) || defined(__OpenBSD__)
+#ifdef __sun__
+/* Once Solaris has openpty(), this is going to be removed. */
+int openpty(int *amaster, int *aslave, char *name,
+ struct termios *termp, struct winsize *winp)
+{
+ const char *slave;
+ int mfd = -1, sfd = -1;
+
+ *amaster = *aslave = -1;
+
+ mfd = open("/dev/ptmx", O_RDWR | O_NOCTTY);
+ if (mfd < 0)
+ goto err;
+
+ if (grantpt(mfd) == -1 || unlockpt(mfd) == -1)
+ goto err;
+
+ if ((slave = ptsname(mfd)) == NULL)
+ goto err;
+
+ if ((sfd = open(slave, O_RDONLY | O_NOCTTY)) == -1)
+ goto err;
+
+ if (ioctl(sfd, I_PUSH, "ptem") == -1 ||
+ (termp != NULL && tcgetattr(sfd, termp) < 0))
+ goto err;
+
+ if (amaster)
+ *amaster = mfd;
+ if (aslave)
+ *aslave = sfd;
+ if (winp)
+ ioctl(sfd, TIOCSWINSZ, winp);
+
+ return 0;
+
+err:
+ if (sfd != -1)
+ close(sfd);
+ close(mfd);
+ return -1;
+}
+
+void cfmakeraw (struct termios *termios_p)
+{
+ termios_p->c_iflag &=
+ ~(IGNBRK|BRKINT|PARMRK|ISTRIP|INLCR|IGNCR|ICRNL|IXON);
+ termios_p->c_oflag &= ~OPOST;
+ termios_p->c_lflag &= ~(ECHO|ECHONL|ICANON|ISIG|IEXTEN);
+ termios_p->c_cflag &= ~(CSIZE|PARENB);
+ termios_p->c_cflag |= CS8;
+
+ termios_p->c_cc[VMIN] = 0;
+ termios_p->c_cc[VTIME] = 0;
+}
+
+#endif
+
+#if defined(__linux__) || defined(__NetBSD__) || defined(__OpenBSD__) ||
defined(__sun__)
static CharDriverState *qemu_chr_open_pty(void)
{
struct termios tty;
@@ -1816,6 +1877,8 @@ static CharDriverState *qemu_chr_open_pt
cfmakeraw(&tty);
tcsetattr(slave_fd, TCSAFLUSH, &tty);
+ close(slave_fd);
+
fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
return qemu_chr_open_fd(master_fd, master_fd);
@@ -2038,7 +2101,7 @@ static CharDriverState *qemu_chr_open_pt
{
return NULL;
}
-#endif /* __linux__ || __NetBSD__ || __OpenBSD__ */
+#endif /* __linux__ || __NetBSD__ || __OpenBSD__ || __sun__ */
#endif /* !defined(_WIN32) */
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |