[PATCH] Fix xenconsole's "Could not read tty from store"

[PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

Hello,

I was getting intermittent "Could not read tty from store" when creating
domains. This is because tools/console/daemon/io.c:domain_create_tty()
seems to expect openpty() to initiaze term, but it's the converse:
openpty expects to be given term parameters and doesn't touch it, so
that term mostly contains random data when given to tcsetattr, and thus
console creation failure. Here is a patch that fixes this.

Samuel

Use tcgetattr to fetch the initial terminal parameters in the console
daemon.

Signed-off-by: Samuel Thibault <samuel.thibault@citrix.com>

diff -r c005937f67d5 tools/console/daemon/io.c
--- a/tools/console/daemon/io.c	Mon Dec 17 10:55:33 2007 +0000
+++ b/tools/console/daemon/io.c	Mon Dec 17 11:17:39 2007 +0000
@@ -282,7 +282,7 @@ static int domain_create_tty(struct doma
 	char *data;
 	unsigned int len;
 
-	if (openpty(&master, &slavefd, slave, &term, NULL) < 0) {
+	if (openpty(&master, &slavefd, slave, NULL, NULL) < 0) {
 		master = -1;
 		err = errno;
 		dolog(LOG_ERR, "Failed to create tty for domain-%d (errno = %i, %s)",
@@ -290,10 +290,16 @@ static int domain_create_tty(struct doma
 		return master;
 	}
 
+	if (tcgetattr(master, &term) < 0) {
+		err = errno;
+		dolog(LOG_ERR, "Failed to get tty attribute for domain-%d (errno = %i, %s)",
+		      dom->domid, err, strerror(err));
+		goto out;
+	}
 	cfmakeraw(&term);
 	if (tcsetattr(master, TCSAFLUSH, &term) < 0) {
 		err = errno;
-		dolog(LOG_ERR, "Failed to set tty attribute  for domain-%d (errno = %i, %s)",
+		dolog(LOG_ERR, "Failed to set tty attribute for domain-%d (errno = %i, %s)",
 		      dom->domid, err, strerror(err));
 		goto out;
 	}

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[John Levon]

On Mon, Dec 17, 2007 at 11:22:54AM +0000, Samuel Thibault wrote:

> I was getting intermittent "Could not read tty from store" when creating
> domains. This is because tools/console/daemon/io.c:domain_create_tty()
> seems to expect openpty() to initiaze term, but it's the converse:
> openpty expects to be given term parameters and doesn't touch it, so
> that term mostly contains random data when given to tcsetattr, and thus
> console creation failure. Here is a patch that fixes this.

I already sent out a patch for this very code to the list for testing
last week. Please can I get some testing of that patch??

It'll need merging with Samuel's patch I suppose. Note that we don't see
his problem on Solaris as the openpty() implementation above gets the
semantics wrong and we get away with it.

> +	if (tcgetattr(master, &term) < 0) {
> +		err = errno;

Please use the slave not the master. It doesn't matter on Linux but it's
critical on Solaris.

If it helps I can send out an updated patch merging these fixes. Will
you test it if so Samuel?

regards
john

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

John Levon, le Mon 17 Dec 2007 16:42:46 +0000, a écrit :
> > +	if (tcgetattr(master, &term) < 0) {
> > +		err = errno;
> 
> Please use the slave not the master. It doesn't matter on Linux but it's
> critical on Solaris.

Ok. 

> If it helps I can send out an updated patch merging these fixes. Will
> you test it if so Samuel?

Sure!

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[John Levon]

On Mon, Dec 17, 2007 at 04:54:01PM +0000, Samuel Thibault wrote:

> > If it helps I can send out an updated patch merging these fixes. Will
> > you test it if so Samuel?

See below. Thanks a lot.

regards
john


# HG changeset patch
# User john.levon@sun.com
# Date 1197910843 28800
# Node ID 6490ff3cf622271793c7c7f48cb33508084e4d9e
# Parent  9011d85ef535ce57ccfb459f42e3483db4dad3da
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
- a read of zero from the master means that a slave closed, and
  is not an error
- openpty() sets, not gets, terminal settings

Signed-off-by: John Levon <john.levon@sun.com>
Signed-off-by: Samuel Thibault <samuel.thibault@citrix.com>

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))
@@ -230,50 +234,66 @@ static int create_domain_log(struct doma
 #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)
+            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;
 }
+
+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
-
 
 static int domain_create_tty(struct domain *dom)
 {
-	char slave[80];
+	const char *slave;
 	struct termios term;
 	char *path;
 	int master, slavefd;
@@ -282,7 +302,7 @@ static int domain_create_tty(struct doma
 	char *data;
 	unsigned int len;
 
-	if (openpty(&master, &slavefd, slave, &term, NULL) < 0) {
+	if (openpty(&master, &slavefd, NULL, NULL, NULL) < 0) {
 		master = -1;
 		err = errno;
 		dolog(LOG_ERR, "Failed to create tty for domain-%d (errno = %i, %s)",
@@ -290,14 +310,30 @@ static int domain_create_tty(struct doma
 		return master;
 	}
 
-	cfmakeraw(&term);
-	if (tcsetattr(master, TCSAFLUSH, &term) < 0) {
+	if ((slave = ptsname(master)) == 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 (tcgetattr(slavefd, &term) < 0) {
+	        err = errno;
+	        dolog(LOG_ERR, "Failed to get tty attribute for domain-%d (errno = %i, %s)",
+	              dom->domid, err, strerror(err));
+	        goto out;
+	}
+
+	cfmakeraw(&term);
+
+	if (tcsetattr(slavefd, TCSAFLUSH, &term) < 0) {
+		err = errno;
+		dolog(LOG_ERR, "Failed to set tty attribute for domain-%d (errno = %i, %s)",
+		      dom->domid, err, strerror(err));
+		goto out;
+	}
+
+	close(slavefd);
 
 	if (dom->use_consolepath) {
 		success = asprintf(&path, "%s/limit", dom->conspath) !=
@@ -684,7 +720,9 @@ static void handle_tty_read(struct domai
 		len = sizeof(msg);
 
 	len = read(dom->tty_fd, msg, len);
-	if (len < 1) {
+	if (len == 0) {
+		/* slave did a close: that's fine. */
+	} else if (len < 0) {
 		close(dom->tty_fd);
 		dom->tty_fd = -1;
 
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) */

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

John Levon, le Tue 18 Dec 2007 01:41:44 +0000, a écrit :
> On Mon, Dec 17, 2007 at 04:54:01PM +0000, Samuel Thibault wrote:
> 
> > > If it helps I can send out an updated patch merging these fixes. Will
> > > you test it if so Samuel?
> 
> See below. Thanks a lot.

Seems to work fine!

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[John Levon]

On Tue, Dec 18, 2007 at 01:41:44AM +0000, John Levon wrote:

> On Mon, Dec 17, 2007 at 04:54:01PM +0000, Samuel Thibault wrote:
> 
> > > If it helps I can send out an updated patch merging these fixes. Will
> > > you test it if so Samuel?
> 
> See below. Thanks a lot.

Keir, Samuel has tested the below patch and verifies it works on Linux.
Please apply.

thanks
john


# HG changeset patch
# User john.levon@sun.com
# Date 1197910843 28800
# Node ID 6490ff3cf622271793c7c7f48cb33508084e4d9e
# Parent  9011d85ef535ce57ccfb459f42e3483db4dad3da
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
- a read of zero from the master means that a slave closed, and
  is not an error
- openpty() sets, not gets, terminal settings

Signed-off-by: John Levon <john.levon@sun.com>
Signed-off-by: Samuel Thibault <samuel.thibault@citrix.com>

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))
@@ -230,50 +234,66 @@ static int create_domain_log(struct doma
 #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)
+            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;
 }
+
+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
-
 
 static int domain_create_tty(struct domain *dom)
 {
-	char slave[80];
+	const char *slave;
 	struct termios term;
 	char *path;
 	int master, slavefd;
@@ -282,7 +302,7 @@ static int domain_create_tty(struct doma
 	char *data;
 	unsigned int len;
 
-	if (openpty(&master, &slavefd, slave, &term, NULL) < 0) {
+	if (openpty(&master, &slavefd, NULL, NULL, NULL) < 0) {
 		master = -1;
 		err = errno;
 		dolog(LOG_ERR, "Failed to create tty for domain-%d (errno = %i, %s)",
@@ -290,14 +310,30 @@ static int domain_create_tty(struct doma
 		return master;
 	}
 
-	cfmakeraw(&term);
-	if (tcsetattr(master, TCSAFLUSH, &term) < 0) {
+	if ((slave = ptsname(master)) == 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 (tcgetattr(slavefd, &term) < 0) {
+	        err = errno;
+	        dolog(LOG_ERR, "Failed to get tty attribute for domain-%d (errno = %i, %s)",
+	              dom->domid, err, strerror(err));
+	        goto out;
+	}
+
+	cfmakeraw(&term);
+
+	if (tcsetattr(slavefd, TCSAFLUSH, &term) < 0) {
+		err = errno;
+		dolog(LOG_ERR, "Failed to set tty attribute for domain-%d (errno = %i, %s)",
+		      dom->domid, err, strerror(err));
+		goto out;
+	}
+
+	close(slavefd);
 
 	if (dom->use_consolepath) {
 		success = asprintf(&path, "%s/limit", dom->conspath) !=
@@ -684,7 +720,9 @@ static void handle_tty_read(struct domai
 		len = sizeof(msg);
 
 	len = read(dom->tty_fd, msg, len);
-	if (len < 1) {
+	if (len == 0) {
+		/* slave did a close: that's fine. */
+	} else if (len < 0) {
 		close(dom->tty_fd);
 		dom->tty_fd = -1;
 
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) */

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

Oops, there may be a problem with this:

John Levon, le Tue 18 Dec 2007 01:41:44 +0000, a écrit :
> @@ -684,7 +720,9 @@ static void handle_tty_read(struct domai
>  		len = sizeof(msg);
>  
>  	len = read(dom->tty_fd, msg, len);
> -	if (len < 1) {
> +	if (len == 0) {
> +		/* slave did a close: that's fine. */
> +	} else if (len < 0) {
>  		close(dom->tty_fd);
>  		dom->tty_fd = -1;
>  

If the slave did a close, you have to somehow remove the fd from the
select, because else you'll loop trying to read EOF.

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

John Levon, le Tue 18 Dec 2007 13:35:22 +0000, a écrit :
> On Tue, Dec 18, 2007 at 01:41:44AM +0000, John Levon wrote:
> 
> > On Mon, Dec 17, 2007 at 04:54:01PM +0000, Samuel Thibault wrote:
> > 
> > > > If it helps I can send out an updated patch merging these fixes. Will
> > > > you test it if so Samuel?
> > 
> > See below. Thanks a lot.
> 
> Keir, Samuel has tested the below patch and verifies it works on Linux.
> Please apply.

Hermf, actually no, I wonder why I didn't get the issue before. You
close the slave part of the pty, but on linux it looks like that makes
the master part stop working: I'm getting EIO on read()s, so that
xenconsoled tries recreating it ad aeternam, while xend doesn't manage
to start the domain...

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[John Levon]

[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]

On Tue, Dec 18, 2007 at 05:03:59PM +0000, Samuel Thibault wrote:

> Oops, there may be a problem with this:
> 
> John Levon, le Tue 18 Dec 2007 01:41:44 +0000, a écrit :
> > @@ -684,7 +720,9 @@ static void handle_tty_read(struct domai
> >  		len = sizeof(msg);
> >  
> >  	len = read(dom->tty_fd, msg, len);
> > -	if (len < 1) {
> > +	if (len == 0) {
> > +		/* slave did a close: that's fine. */
> > +	} else if (len < 0) {
> >  		close(dom->tty_fd);
> >  		dom->tty_fd = -1;
> >  
> 
> If the slave did a close, you have to somehow remove the fd from the
> select, because else you'll loop trying to read EOF.

Not on Solaris:

$ ./pty
terminal is /dev/pts/35
select returned
got 0 read: slave was closed
<waits>

See attached pty.c. On Linux I get:

$ ./pty
terminal is /dev/pts/9
select returned
read of master: -1 5
select returned
read of master: -1 5
select returned
...

It looks like we must keep the slave fd open on both platforms instead,
as Linux's behaviour if we close the slave is unusable. Do you agree?

regards
john

[-- Attachment #2: pty.c --]
[-- Type: text/plain, Size: 3587 bytes --]

#define _GNU_SOURCE
#include <stdlib.h>
#include <stdio.h>
#include <stropts.h>
#include <fcntl.h>
#include <termios.h>
#include <errno.h>
#include <sys/time.h>
#include <pty.h>


#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, sfd = -1;

        *amaster = *aslave = -1;
        mfd = sfd = -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) {
                fprintf(stderr, "foo\n");
                goto err;
        }

        sfd = open(slave, O_RDONLY | O_NOCTTY);

        if (sfd == -1) {
                fprintf(stderr, "failed open\n");
                goto err;
        }

        if (ioctl(sfd, I_PUSH, "ptem") == -1 ||
            (termp != NULL && tcgetattr(sfd, termp) < 0)) {
                fprintf(stderr, "failed!");
                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

int main()
{
        const char *slave;
        struct termios term;
        int master, slavefd;
        char buf[1];
        fd_set readfds;

        if (openpty(&master, &slavefd, NULL, &term, NULL) < 0) {
                fprintf(stderr, "failed openpty %d\n", errno);
                exit(1);
        }

        slave = ptsname(master);
        printf("terminal is %s\n", slave);
        fflush(stdout);

        if (fcntl(master, F_SETFL, O_NONBLOCK) == -1) {
                fprintf(stderr, "failed to set non block\n");
                exit(1);
        }

        cfmakeraw(&term);

        if (tcsetattr(slavefd, TCSAFLUSH, &term) < 0) {
                fprintf(stderr, "tcsetattr failed %d\n", errno);
                exit(1);
        }

        close(slavefd);


        for (;;) {
                int ret;

                FD_ZERO(&readfds);
                FD_SET(master, &readfds);
                ret = select(master + 1, &readfds, NULL,NULL, NULL);

                if (ret == -1) {
                        fprintf(stderr, "select got -1 %d\n", errno);
                } else {
                        fprintf(stderr, "select returned\n");
                }

                if (!FD_ISSET(master, &readfds)) {
                        fprintf(stderr, "huh? master isn't set?\n");
                }

                ret = read(master, buf, 1);

                if (ret == 0) {
                        fprintf(stderr, "got 0 read: slave was closed\n");
                } else if (ret > 0) {
                        fprintf(stderr, "got %d bytes from master: %c\n", ret, buf[0]);
                } else if (ret != -1 || errno != EAGAIN) {
                        fprintf(stderr, "read of master: %d %d\n", ret, errno);
                }
        }
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

John Levon, le Mon 17 Dec 2007 16:42:46 +0000, a écrit :
> > +	if (tcgetattr(master, &term) < 0) {
> > +		err = errno;
> 
> Please use the slave not the master. It doesn't matter on Linux but it's
> critical on Solaris.

What is the Solaris reason for this?  It really looks we can't afford
closing the slave end on Linux (hence leak it) or not open it at all
(and then we can't tcgetattr on it).

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[John Levon]

On Tue, Dec 18, 2007 at 06:07:17PM +0000, Samuel Thibault wrote:

> > > +	if (tcgetattr(master, &term) < 0) {
> > > +		err = errno;
> > 
> > Please use the slave not the master. It doesn't matter on Linux but it's
> > critical on Solaris.
> 
> What is the Solaris reason for this?  It really looks we can't afford
> closing the slave end on Linux (hence leak it) or not open it at all
> (and then we can't tcgetattr on it).

On Linux, both ends of a pty act like a pseudo-terminal and you can do
term-like things on both ends (and there's apparently small variations
in the effect they have). This is not true on STREAMS-based systems. In
particular, you have to push the "ptem" module before you can use any of
the tcget/setattr() functions (look at openpty()).

I'm working on a new version of the patch that just keeps the slave open
(without leaking it) on both platforms - I think this should cover these
differences sufficiently.

regards
john

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

John Levon, le Tue 18 Dec 2007 18:13:53 +0000, a écrit :
> On Tue, Dec 18, 2007 at 06:07:17PM +0000, Samuel Thibault wrote:
> 
> > > > +	if (tcgetattr(master, &term) < 0) {
> > > > +		err = errno;
> > > 
> > > Please use the slave not the master. It doesn't matter on Linux but it's
> > > critical on Solaris.
> > 
> > What is the Solaris reason for this?  It really looks we can't afford
> > closing the slave end on Linux (hence leak it) or not open it at all
> > (and then we can't tcgetattr on it).
> 
> On Linux, both ends of a pty act like a pseudo-terminal and you can do
> term-like things on both ends (and there's apparently small variations
> in the effect they have). This is not true on STREAMS-based systems. In
> particular, you have to push the "ptem" module before you can use any of
> the tcget/setattr() functions (look at openpty()).

I'm precisely reading that part of the sun manual thanks to google, yes
:)

Can't we push those modules on such systems?

> I'm working on a new version of the patch that just keeps the slave open
> (without leaking it) on both platforms - I think this should cover these
> differences sufficiently.

Well, there should be no reason to open the slave part at all, actually.
To my understanding, from the server side tcsetattr should only be
performed on the master side (with effect on the slave side too).

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[John Levon]

On Tue, Dec 18, 2007 at 06:18:16PM +0000, Samuel Thibault wrote:

> > > > > +	if (tcgetattr(master, &term) < 0) {
> > > > > +		err = errno;
> > > > 
> > > > Please use the slave not the master. It doesn't matter on Linux but it's
> > > > critical on Solaris.
> > > 
> > > What is the Solaris reason for this?  It really looks we can't afford
> > > closing the slave end on Linux (hence leak it) or not open it at all
> > > (and then we can't tcgetattr on it).
> > 
> > On Linux, both ends of a pty act like a pseudo-terminal and you can do
> > term-like things on both ends (and there's apparently small variations
> > in the effect they have). This is not true on STREAMS-based systems. In
> > particular, you have to push the "ptem" module before you can use any of
> > the tcget/setattr() functions (look at openpty()).
> 
> I'm precisely reading that part of the sun manual thanks to google, yes
> :)
> 
> Can't we push those modules on such systems?

I do - on the slave. It's nonsensical to push it onto the master, I
think. In fact it's dubious doing this tcsetattr() /at all/ - I would
have thought that xenconsole is the right place to be doing that.

> > I'm working on a new version of the patch that just keeps the slave open
> > (without leaking it) on both platforms - I think this should cover these
> > differences sufficiently.
> 
> Well, there should be no reason to open the slave part at all, actually.

openpty() always gives us a slave fd so it's already open. There's no way
on Linux to not busy-loop without keeping a slave fd open, since you set
non-blocking on the master fd. Additionally, if we were to close the
slave fd, then we have to re-introduce the code that checks for a read
of zero for Solaris.

> To my understanding, from the server side tcsetattr should only be
> performed on the master side (with effect on the slave side too).

See above, I don't quite follow why it's necessary at all.

regards
john

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

John Levon, le Tue 18 Dec 2007 18:24:54 +0000, a écrit :
> On Tue, Dec 18, 2007 at 06:18:16PM +0000, Samuel Thibault wrote:
> > > > > > +	if (tcgetattr(master, &term) < 0) {
> > > > > > +		err = errno;
> > > > > 
> > > > > Please use the slave not the master. It doesn't matter on Linux but it's
> > > > > critical on Solaris.
> > > > 
> > > > What is the Solaris reason for this?  It really looks we can't afford
> > > > closing the slave end on Linux (hence leak it) or not open it at all
> > > > (and then we can't tcgetattr on it).
> > > 
> > > On Linux, both ends of a pty act like a pseudo-terminal and you can do
> > > term-like things on both ends (and there's apparently small variations
> > > in the effect they have). This is not true on STREAMS-based systems. In
> > > particular, you have to push the "ptem" module before you can use any of
> > > the tcget/setattr() functions (look at openpty()).
> > 
> > I'm precisely reading that part of the sun manual thanks to google, yes
> > :)
> > 
> > Can't we push those modules on such systems?
> 
> I do - on the slave. It's nonsensical to push it onto the master, I
> think. In fact it's dubious doing this tcsetattr() /at all/ - I would
> have thought that xenconsole is the right place to be doing that.

Then I guess it's the proper way to go.

> > > I'm working on a new version of the patch that just keeps the slave open
> > > (without leaking it) on both platforms - I think this should cover these
> > > differences sufficiently.
> > 
> > Well, there should be no reason to open the slave part at all, actually.
> 
> openpty() always gives us a slave fd so it's already open.

Right, but we can avoid using openpty(), but posix_openpt() or
open("/dev/ptmx"). On linux that works fine.

> There's no way on Linux to not busy-loop without keeping a slave fd
> open, since you set non-blocking on the master fd.

If the slave fd is not opened at all as suggested above, there should be
no loop.

> > To my understanding, from the server side tcsetattr should only be
> > performed on the master side (with effect on the slave side too).
> 
> See above, I don't quite follow why it's necessary at all.

That was my understanding of how pseudo terminals are supposed to work,
i.e. the master part is only for the server and the slave part is only
for client, but who is responsible for calling tcsetattr() is another
story, and as said above the best could be to let the client do it.

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

Samuel Thibault, le Tue 18 Dec 2007 18:47:50 +0000, a écrit :
> > I do - on the slave. It's nonsensical to push it onto the master, I
> > think. In fact it's dubious doing this tcsetattr() /at all/ - I would
> > have thought that xenconsole is the right place to be doing that.
> 
> Then I guess it's the proper way to go.

And I could test on both linux and solaris, it works fine.

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[John Levon]

On Tue, Dec 18, 2007 at 06:18:16PM +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? My test program works
without needing the tcsetattr, as does xenconsoled

thanks
john


# HG changeset patch
# User john.levon@sun.com
# Date 1198003657 28800
# Node ID 6f03f4ec458d4780314c015da214795b7b1cf195
# Parent  539cbabd97b5ff3d335de151636040bb2f4cd629
Fix master/slave handling in xenconsoled

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@sun.com>
Signed-off-by: Samuel Thibault <samuel.thibault@citrix.com>

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)

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[John Levon]

On Tue, Dec 18, 2007 at 06:47:50PM +0000, Samuel Thibault wrote:

> > openpty() always gives us a slave fd so it's already open.
> 
> Right, but we can avoid using openpty(), but posix_openpt() or
> open("/dev/ptmx"). On linux that works fine.

We could, but that runs the risk of breaking BSD again. Christoph is
away at the moment too. I think keeping the slave open is reasonable and
least chance of hurting us.

> > There's no way on Linux to not busy-loop without keeping a slave fd
> > open, since you set non-blocking on the master fd.
> 
> If the slave fd is not opened at all as suggested above, there should be
> no loop.

I'm not sure we'd always get the right behaviour when it's opened and
then closed again...

Anyway, see my next attempt at a fix :)

regards
john

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

John Levon, le Tue 18 Dec 2007 18:52:11 +0000, a écrit :
> On Tue, Dec 18, 2007 at 06:18:16PM +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? My test program works
> without needing the tcsetattr, as does xenconsoled

I guess xenconsoled needs tcsetattr, since else you'd get all the tty
interpretation which interacts badly with what guests need for the
console.

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[John Levon]

On Tue, Dec 18, 2007 at 07:00:24PM +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? My test program works
> > without needing the tcsetattr, as does xenconsoled
> 
> I guess xenconsoled needs tcsetattr, since else you'd get all the tty
> interpretation which interacts badly with what guests need for the
> console.

I'm not sure what you mean, but this doesn't happen on Solaris (ie it
works fine for me without)

regards
john

VT-d and the GPU

[Philip Kufeldt]

Like others on this list I am trying to employ the VTD-NEO patches in 
Xen 3.2 unstable to assign the internal graphics device to Dom1/Vista.  
I have removed the Cirrus Logic emulated device from qemu and replaced 
the Cirrus Logic vgabios with the the actual vgabios from my GPU.  
However I am hitting an xen assert  and was hoping someone might be able 
to point me in the right direction. Below is the  console output from 
from the the xm create  to the assert.

BTW: I have noted the missing bios call when posting the the new vgabios 
and if any one knows the details surrounding int15h AX=5F21, I would 
also appreciate  that as well, I know it has something to do with the 
GPU clock rate.

    (XEN) intel-iommu.c:1965:d0 iommu_set_pgd: hd->pgd = ff25f000
    (XEN) intel-iommu.c:1843: assign_device: bus = 0 dev = 2 func = 0
    (XEN) intel-iommu.c:1371:d0 reassign_device-0:2:0- source = 0 target = 1
    (XEN) dmar.c:110:d0 acpi_find_matched_drhd_unit: drhd->address =
    feb01000
    (XEN) intel-iommu.c:1312:d0 domain_context_unmap:PCI: bdf = 0:2:0
    (XEN) intel-iommu.c:1260:d0 domain_context_unmap_one_1:bdf = 0:2:0
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8000002
    (XEN) print_vtd_entries: domain_id = 0 bdf = 0:2:0 devfn = 10, gmfn
    = 7e800
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     d->pgd = ffbce000 virt_to_maddr(hd->pgd) = bce000
    (XEN)     root_entry = ffbcb000
    (XEN)     root_entry[0] = bc5001
    (XEN)     maddr_to_virt(root_entry[0]) = ffbc5001
    (XEN)     ctxt_entry[10].lo == 0
    (XEN) intel-iommu.c:1271:d0 domain_context_unmap_one_2:bdf = 0:2:0
    (XEN) intel-iommu.c:1186:d0 domain_context_mapping:PCI: bdf = 0:2:0
    (XEN) domctl.c:552:d0 XEN_DOMCTL_assign_device: bdf = 0:2:0
    (XEN) io.c:115:d0 XEN_DOMCTL_irq_mapping: m_irq = 14 device = 2 intx = 0
    (XEN) io.c:157:d0 hvm_dpci_eoi:: device 2 intx 0
    (XEN) io.c:157:d32767 hvm_dpci_eoi:: device 2 intx 0
    (XEN) io.c:157:d32767 hvm_dpci_eoi:: device 2 intx 0
    (XEN) HVM1: HVM Loader
    (XEN) HVM1: Detected Xen v3.2-unstable
    (XEN) HVM1: Writing SMBIOS tables ...
    (XEN) HVM1: Loading ROMBIOS ...
    (XEN) HVM1: 8972 bytes of ROMBIOS high-memory extensions:
    (XEN) io.c:157:d32767 hvm_dpci_eoi:: device 2 intx 0
    (XEN) HVM1:   Relocating to 0x3fffac00-0x3fffcf0c ... done
    (XEN) irq.c:222: Dom1 PCI link 0 changed 0 -> 5
    (XEN) HVM1: PCI-ISA link 0 routed to IRQ5
    (XEN) irq.c:222: Dom1 PCI link 1 changed 0 -> 10
    (XEN) HVM1: PCI-ISA link 1 routed to IRQ10
    (XEN) irq.c:222: Dom1 PCI link 2 changed 0 -> 11
    (XEN) HVM1: PCI-ISA link 2 routed to IRQ11
    (XEN) irq.c:222: Dom1 PCI link 3 changed 0 -> 5
    (XEN) HVM1: PCI-ISA link 3 routed to IRQ5
    (XEN) HVM1: pci dev 01:1 bar 20 size 00000010: 0000c001
    (XEN) io.c:157:d32767 hvm_dpci_eoi:: device 2 intx 0
    (XEN) HVM1: pci dev 01:2 INTA->IRQ10
    (XEN) HVM1: pci dev 02:0 bar 10 size 00080000: f0000000
    (XEN) domctl.c:595:d0 memory_map:add: gfn=f0000 mfn=90480 nr_mfns=80
    (XEN) HVM1: pci dev 02:0 bar 14 size 00000008: 0000c011
    (XEN) domctl.c:646:d0 ioport_map:add f_gport=c010 f_mport=3410 np=8
    (XEN) HVM1: pci dev 02:0 bar 18 size 10000000: 00000008
    (XEN) domctl.c:595:d0 memory_map:add: gfn=10000 mfn=90300 nr_mfns=100
    (XEN) HVM1: pci dev 02:0 bar 1c size 00100000: 10000000
    (XEN) HVM1: pci dev 02:0 INTA->IRQ11
    (XEN) HVM1: pci dev 03:0 bar 10 size 00000100: 0000c101
    (XEN) HVM1: pci dev 03:0 bar 14 size 01000000: 11000008
    (XEN) HVM1: pci dev 03:0 INTA->IRQ5
    (XEN) HVM1: pci dev 04:0 bar 10 size 00000100: 0000c201
    (XEN) HVM1: pci dev 04:0 bar 14 size 00000100: 12000000
    (XEN) HVM1: pci dev 04:0 INTA->IRQ5
    (XEN) HVM1: Creating MP tables ...
    (XEN) HVM1: Loading INtel GMA 3100 VGABIOS ...
    (XEN) HVM1: Loading ACPI ...
    (XEN) HVM1: BIOS map:
    (XEN) HVM1:  c0000-cb3ff: VGA BIOS
    (XEN) HVM1:  d0000-d5fff: VMXAssist
    (XEN) HVM1:  e9000-e9143: SMBIOS tables
    (XEN) HVM1:  ea000-eb18f: ACPI tables
    (XEN) HVM1:  f0000-fffff: Main BIOS
    (XEN) HVM1: Loading VMXAssist ...
    (XEN) HVM1: VMX go ...
    (XEN) HVM1: VMXAssist (Dec 11 2007)
    (XEN) HVM1: Memory size 1023 MB
    (XEN) HVM1: E820 map:
    (XEN) HVM1: 0000000000000000 - 000000000009FC00 (RAM)
    (XEN) HVM1: 000000000009FC00 - 00000000000A0000 (Reserved)
    (XEN) HVM1: 00000000000E0000 - 0000000000100000 (Reserved)
    (XEN) HVM1: 0000000000100000 - 000000003FFFAC00 (RAM)
    (XEN) HVM1: 000000003FFFAC00 - 0000000040000000 (Reserved)
    (XEN) HVM1:
    (XEN) HVM1: Start BIOS ...
    (XEN) HVM1: Starting emulated 16-bit real-mode: ip=F000:FFF0
    (XEN) HVM1:  rombios.c,v 1.138 2005/05/07 15:55:26 vruppert Exp $
    (XEN) io.c:157:d32767 hvm_dpci_eoi:: device 2 intx 0
    (XEN) HVM1: Remapping master: ICW2 0x8 -> 0x20
    (XEN) HVM1: Remapping slave: ICW2 0x70 -> 0x28
    (XEN) HVM1: *** int 15h function AX=5F21, BX=C000 not yet supported!
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8000005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e800
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8010005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e801
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8040005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e804
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8050005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e805
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8060005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e806
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8090005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e809
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e80a0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e80a
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e80b0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e80b
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e80e0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e80e
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e80f0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e80f
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8100005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e810
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8130005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e813
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8140005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e814
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8150005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e815
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8180005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e818
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8190005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e819
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e81a0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e81a
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e81d0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e81d
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e81e0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e81e
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e81f0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e81f
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8200005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e820
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8230005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e823
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8240005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e824
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8270005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e827
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8280005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e828
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e82b0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e82b
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e82c0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e82c
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e82d0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e82d
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e82e0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e82e
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8310005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e831
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8320005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e832
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8350005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e835
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8360005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e836
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8390005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e839
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e83a0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e83a
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e83b0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e83b
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e83c0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e83c
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e83f0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e83f
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8400005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e840
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8430005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e843
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8440005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e844
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8470005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e847
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8480005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e848
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8490005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e849
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e84a0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e84a
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e84c0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e84c
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e84d0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e84d
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8500005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e850
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8510005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e851
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8520005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e852
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8550005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e855
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8560005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e856
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8570005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e857
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e85a0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e85a
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e85b0005
    (XEN) print_vtd_entries: domain_id = 7fff bdf = 0:2:0 devfn = 10,
    gmfn = 7e85b
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     hg->pgd == NULL
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8fe0005
    (XEN) print_vtd_entries: domain_id = 0 bdf = 0:2:0 devfn = 10, gmfn
    = 7e8fe
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     d->pgd = ffbce000 virt_to_maddr(hd->pgd) = bce000
    (XEN)     root_entry = ffbcb000
    (XEN)     root_entry[0] = bc5001
    (XEN)     maddr_to_virt(root_entry[0]) = ffbc5001
    (XEN)     ctxt_entry[10].lo == 0
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8fe0005
    (XEN) print_vtd_entries: domain_id = 0 bdf = 0:2:0 devfn = 10, gmfn
    = 7e8fe
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     d->pgd = ffbce000 virt_to_maddr(hd->pgd) = bce000
    (XEN)     root_entry = ffbcb000
    (XEN)     root_entry[0] = bc5001
    (XEN)     maddr_to_virt(root_entry[0]) = ffbc5001
    (XEN)     ctxt_entry[10].lo == 0
    (XEN) intel-iommu.c:741: iommu_page_fault: iommu->reg = fff77000
    (XEN) intel-iommu.c:724: iommu_page_fault:DMA Write: DEVICE 0:2.0
    addr 7e8fe0005
    (XEN) print_vtd_entries: domain_id = 0 bdf = 0:2:0 devfn = 10, gmfn
    = 7e8fe
    (XEN) ---- print_vtd_entries 0 ----
    (XEN)     d->pgd = ffbce000 virt_to_maddr(hd->pgd) = bce000
    (XEN)     root_entry = ffbcb000
    (XEN)     root_entry[0] = bc5001
    (XEN)     maddr_to_virt(root_entry[0]) = ffbc5001
    (XEN)     ctxt_entry[10].lo == 0
    (XEN) io.c:157:d0 hvm_dpci_eoi:: device 2 intx 0
    (XEN) Assertion '(sp == 0) || (peoi[sp-1].vector < vector)' failed
    at irq.c:226
    (XEN) ----[ Xen-3.2-unstable  x86_32p  debug=y  Not tainted ]----
    (XEN) CPU:    0
    (XEN) EIP:    e008:[<ff12ab6b>] do_IRQ+0xe8/0x299
    (XEN) EFLAGS: 00010002   CONTEXT: hypervisor
    (XEN) eax: 00000001   ebx: 00000001   ecx: ff1c8200   edx: 00000001
    (XEN) esi: ff274580   edi: 000000d8   ebp: ff1ebe9c   esp: ff1ebe74
    (XEN) cr0: 8005003b   cr4: 000026f0   cr3: 00bccd00   cr2: b74fd1f4
    (XEN) ds: e010   es: e010   fs: 0000   gs: 0000   ss: e010   cs: e008
    (XEN) Xen stack trace from esp=ff1ebe74:
    (XEN)    000000d8 ff1a5c20 ff1ebe9c ff208300 00000000 00000014
    ff1c83fe 00000000
    (XEN)    0000e010 0000e010 00e14143 ff1267e6 ff1ebea8 00000000
    0000000a 00000000
    (XEN)    00000286 00000014 ff1ebefc 00000000 00d80000 ff11fa41
    0000e008 00000286
    (XEN)    ff1fac31 0000000a ff1a1d25 ff1ebf1c ffbcc100 00000001
    ff1fac31 ff1ebf08
    (XEN)    00000000 00000002 ff1ebf2c ff16e2c9 ff1a1cf4 ff196bba
    0000009d 00000000
    (XEN)    00000002 00000000 4f742ed4 00000018 ff16e307 ff1c8100
    ff1ebf4c ff16e348
    (XEN)    ff272100 00000018 00000000 ff2570c0 ffbf4300 ff2570c0
    ff1ebf8c ff117aba
    (XEN)    ff2541f0 00000010 ff232900 00000001 2e8216f2 00000068
    3079e6b5 00000068
    (XEN)    30792365 00000068 ff12acf1 00000000 ff1ebfb4 0000007b
    ff1ebfac ff116919
    (XEN)    00000000 00000000 f5616020 0000007b ffbcc100 0000007b
    00e14037 ff1887c6
    (XEN)    f5616020 0000037e 0036c00b c1211800 c0495230 c1211840
    6822f85b 00a00000
    (XEN)    c01083df 00000061 00000a03 c0461e30 00000069 0000007b
    0000007b 00000000
    (XEN)    00000000 00000000 ffbcc100
    (XEN) Xen call trace:
    (XEN)    [<ff12ab6b>] do_IRQ+0xe8/0x299
    (XEN)    [<ff1267e6>] common_interrupt+0x56/0x60
    (XEN)    [<ff11fa41>] printk+0x15d/0x164
    (XEN)    [<ff16e2c9>] hvm_dpci_eoi+0xd9/0x117
    (XEN)    [<ff16e348>] pt_irq_time_out+0x41/0x47
    (XEN)    [<ff117aba>] timer_softirq_action+0xaf/0x12f
    (XEN)    [<ff116919>] do_softirq+0x4f/0x66
    (XEN)
    (XEN)
    (XEN) ****************************************
    (XEN) Panic on CPU 0:
    (XEN) Assertion '(sp == 0) || (peoi[sp-1].vector < vector)' failed
    at irq.c:226
    (XEN) ****************************************
    (XEN)
    (XEN) Reboot in five seconds...


-- 
Philip A. Kufeldt	     Those who trade freedom for
Int: pak@windspear.com          security will lose both, 
Tel: 408/206-6689	            and deserve neither.
Fax: 408/532-6008			 -- Ben Franklin

Windspear, LLC
www.windspear.com

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Keir Fraser]

Any conclusion on this one, before I do a second release candidate for
3.2.0?

 -- Keir


On 18/12/07 18:52, "John Levon" <levon@movementarian.org> wrote:

> On Tue, Dec 18, 2007 at 06:18:16PM +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? My test program works
> without needing the tcsetattr, as does xenconsoled
> 
> thanks
> john
> 
> 
> # HG changeset patch
> # User john.levon@sun.com
> # Date 1198003657 28800
> # Node ID 6f03f4ec458d4780314c015da214795b7b1cf195
> # Parent  539cbabd97b5ff3d335de151636040bb2f4cd629
> Fix master/slave handling in xenconsoled
> 
> 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@sun.com>
> Signed-off-by: Samuel Thibault <samuel.thibault@citrix.com>
> 
> 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)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

John Levon, le Tue 18 Dec 2007 19:18:23 +0000, a écrit :
> On Tue, Dec 18, 2007 at 07:00:24PM +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? My test program works
> > > without needing the tcsetattr, as does xenconsoled
> > 
> > I guess xenconsoled needs tcsetattr, since else you'd get all the tty
> > interpretation which interacts badly with what guests need for the
> > console.
> 
> I'm not sure what you mean, but this doesn't happen on Solaris (ie it
> works fine for me without)

Well, actually, as you believed the client does it already, so that the
server doesn't need to do it indeed.

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

Keir Fraser, le Wed 19 Dec 2007 11:29:21 +0000, a écrit :
> Any conclusion on this one, before I do a second release candidate for
> 3.2.0?

John should probably be able to provide a fix that works both on Solaris
and Linux systems.

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

John Levon, le Tue 18 Dec 2007 18:55:50 +0000, a écrit :
> Anyway, see my next attempt at a fix :)

Just to make sure: were you referring to the fix from 18:52:11 +000
yesterday (i.e. 3-4 minutes before posting this), or to another patch
still pending?

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[Samuel Thibault]

John Levon, le Tue 18 Dec 2007 18:52:11 +0000, a écrit :
> On Tue, Dec 18, 2007 at 06:18:16PM +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.

Samuel

Re: [PATCH] Fix xenconsole's "Could not read tty from store"

[John Levon]

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@sun.com
# 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@sun.com>
Signed-off-by: Samuel Thibault <samuel.thibault@citrix.com>

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) */