[Qemu-devel] PATCH: Put Psuedo-TTY in rawmode for char devices

[Qemu-devel] PATCH: Put Psuedo-TTY in rawmode for char devices

[Daniel P. Berrange]

If running a QEMU instance with a serial/parallel device connected to a
Psuedo-TTY, eg  '-serial pty',  every \r\n sequence output by the guest
is getting translated into a \n\n sequence by the TTY layer. So clients
interacting with the serial port via a TTY done get the correct \r\n
sequence and text marches to the right and wraps. This is because the 
TTY is not put into rawmode when QEMU sets it up. 

The following patch is a re-diff of a patch applied to Xen's QEMU code.
It uses cfmakeraw() to ensure the TTY is put into rawmode, thus avoiding
the incorrect \r\n translations. It also switches to tcsetattr() on the
slave_fd instead of master_fd - although this is effectively the same on
Linux, only slave_fd works on Solaris. Finally it stops using the 'name'
arg to openpty() which is a security risk because its buffer size is
undefined. Instead it makes use of the ptsname() function.

Regards,
Daniel.

Index: vl.c
===================================================================
--- vl.c	(revision 4229)
+++ vl.c	(working copy)
@@ -2275,24 +2275,20 @@
 static CharDriverState *qemu_chr_open_pty(void)
 {
     struct termios tty;
-    char slave_name[1024];
     int master_fd, slave_fd;
 
 #if defined(__linux__)
     /* Not satisfying */
-    if (openpty(&master_fd, &slave_fd, slave_name, NULL, NULL) < 0) {
+    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) < 0) {
         return NULL;
     }
 #endif
 
-    /* Disabling local echo and line-buffered output */
-    tcgetattr (master_fd, &tty);
-    tty.c_lflag &= ~(ECHO|ICANON|ISIG);
-    tty.c_cc[VMIN] = 1;
-    tty.c_cc[VTIME] = 0;
-    tcsetattr (master_fd, TCSAFLUSH, &tty);
+    /* Set raw attributes on the pty. */
+    cfmakeraw(&tty);
+    tcsetattr(slave_fd, TCSAFLUSH, &tty);
 
-    fprintf(stderr, "char device redirected to %s\n", slave_name);
+    fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
     return qemu_chr_open_fd(master_fd, master_fd);
 }
 


-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] PATCH: Put Psuedo-TTY in rawmode for char devices

[Daniel P. Berrange]

Re-posting patch for review/inclusion...

On Tue, Apr 22, 2008 at 01:50:57AM +0100, Daniel P. Berrange wrote:
> If running a QEMU instance with a serial/parallel device connected to a
> Psuedo-TTY, eg  '-serial pty',  every \r\n sequence output by the guest
> is getting translated into a \n\n sequence by the TTY layer. So clients
> interacting with the serial port via a TTY done get the correct \r\n
> sequence and text marches to the right and wraps. This is because the 
> TTY is not put into rawmode when QEMU sets it up. 
> 
> The following patch is a re-diff of a patch applied to Xen's QEMU code.
> It uses cfmakeraw() to ensure the TTY is put into rawmode, thus avoiding
> the incorrect \r\n translations. It also switches to tcsetattr() on the
> slave_fd instead of master_fd - although this is effectively the same on
> Linux, only slave_fd works on Solaris. Finally it stops using the 'name'
> arg to openpty() which is a security risk because its buffer size is
> undefined. Instead it makes use of the ptsname() function.

Here is the patch re-diff to apply cleanly to latest SVN checkout of
QEMU codebase.

   Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel

Index: vl.c
===================================================================
--- vl.c	(revision 4291)
+++ vl.c	(working copy)
@@ -2273,24 +2273,20 @@
 static CharDriverState *qemu_chr_open_pty(void)
 {
     struct termios tty;
-    char slave_name[1024];
     int master_fd, slave_fd;
 
 #if defined(__linux__)
     /* Not satisfying */
-    if (openpty(&master_fd, &slave_fd, slave_name, NULL, NULL) < 0) {
+    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) < 0) {
         return NULL;
     }
 #endif
 
-    /* Disabling local echo and line-buffered output */
-    tcgetattr (master_fd, &tty);
-    tty.c_lflag &= ~(ECHO|ICANON|ISIG);
-    tty.c_cc[VMIN] = 1;
-    tty.c_cc[VTIME] = 0;
-    tcsetattr (master_fd, TCSAFLUSH, &tty);
+    /* Set raw attributes on the pty. */
+    cfmakeraw(&tty);
+    tcsetattr(slave_fd, TCSAFLUSH, &tty);
 
-    fprintf(stderr, "char device redirected to %s\n", slave_name);
+    fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
     return qemu_chr_open_fd(master_fd, master_fd);
 }
 


-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] PATCH: Put Psuedo-TTY in rawmode for char devices

[Aurelien Jarno]

On Thu, May 01, 2008 at 06:14:24PM +0100, Daniel P. Berrange wrote:
> Re-posting patch for review/inclusion...
> 
> On Tue, Apr 22, 2008 at 01:50:57AM +0100, Daniel P. Berrange wrote:
> > If running a QEMU instance with a serial/parallel device connected to a
> > Psuedo-TTY, eg  '-serial pty',  every \r\n sequence output by the guest
> > is getting translated into a \n\n sequence by the TTY layer. So clients
> > interacting with the serial port via a TTY done get the correct \r\n
> > sequence and text marches to the right and wraps. This is because the 
> > TTY is not put into rawmode when QEMU sets it up. 
> > 
> > The following patch is a re-diff of a patch applied to Xen's QEMU code.
> > It uses cfmakeraw() to ensure the TTY is put into rawmode, thus avoiding
> > the incorrect \r\n translations. It also switches to tcsetattr() on the
> > slave_fd instead of master_fd - although this is effectively the same on
> > Linux, only slave_fd works on Solaris. Finally it stops using the 'name'
> > arg to openpty() which is a security risk because its buffer size is
> > undefined. Instead it makes use of the ptsname() function.
> 

cfmakeraw() is Linux specific, though also available on most BSD
systems. It is unavailable on Solaris, and probably the same on WIN32.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] PATCH: Put Psuedo-TTY in rawmode for char devices

[Samuel Thibault]

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

Aurelien Jarno, le Sun 04 May 2008 01:08:50 +0200, a écrit :
> On Thu, May 01, 2008 at 06:14:24PM +0100, Daniel P. Berrange wrote:
> > Re-posting patch for review/inclusion...
> > 
> > On Tue, Apr 22, 2008 at 01:50:57AM +0100, Daniel P. Berrange wrote:
> > > If running a QEMU instance with a serial/parallel device connected to a
> > > Psuedo-TTY, eg  '-serial pty',  every \r\n sequence output by the guest
> > > is getting translated into a \n\n sequence by the TTY layer. So clients
> > > interacting with the serial port via a TTY done get the correct \r\n
> > > sequence and text marches to the right and wraps. This is because the 
> > > TTY is not put into rawmode when QEMU sets it up. 
> > > 
> > > The following patch is a re-diff of a patch applied to Xen's QEMU code.
> > > It uses cfmakeraw() to ensure the TTY is put into rawmode, thus avoiding
> > > the incorrect \r\n translations. It also switches to tcsetattr() on the
> > > slave_fd instead of master_fd - although this is effectively the same on
> > > Linux, only slave_fd works on Solaris. Finally it stops using the 'name'
> > > arg to openpty() which is a security risk because its buffer size is
> > > undefined. Instead it makes use of the ptsname() function.
> > 
> 
> cfmakeraw() is Linux specific, though also available on most BSD
> systems. It is unavailable on Solaris, and probably the same on WIN32.

Indeed, in Xen we keep a local definition of cfmakeraw for the solaris
case.  That being said, the same lines as term_init could be added, see
attached patch.

Samuel

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 608 bytes --]

Index: vl.c
===================================================================
--- vl.c	(r�vision 4294)
+++ vl.c	(copie de travail)
@@ -2285,7 +2285,11 @@
 
     /* Disabling local echo and line-buffered output */
     tcgetattr (master_fd, &tty);
-    tty.c_lflag &= ~(ECHO|ICANON|ISIG);
+    tty.c_iflag &= ~(IGNBRK|BRKINT|PARMRK|ISTRIP
+                          |INLCR|IGNCR|ICRNL|IXON);
+    tty.c_lflag &= ~(ECHO|ECHONL|ICANON|ISIG|IEXTEN);
+    tty.c_cflag &= ~(CSIZE|PARENB);
+    tty.c_cflag |= CS8;
     tty.c_cc[VMIN] = 1;
     tty.c_cc[VTIME] = 0;
     tcsetattr (master_fd, TCSAFLUSH, &tty);

Re: [Qemu-devel] PATCH: Put Psuedo-TTY in rawmode for char devices

[Daniel P. Berrange]

On Sun, May 04, 2008 at 01:08:50AM +0200, Aurelien Jarno wrote:
> On Thu, May 01, 2008 at 06:14:24PM +0100, Daniel P. Berrange wrote:
> > Re-posting patch for review/inclusion...
> > 
> > On Tue, Apr 22, 2008 at 01:50:57AM +0100, Daniel P. Berrange wrote:
> > > If running a QEMU instance with a serial/parallel device connected to a
> > > Psuedo-TTY, eg  '-serial pty',  every \r\n sequence output by the guest
> > > is getting translated into a \n\n sequence by the TTY layer. So clients
> > > interacting with the serial port via a TTY done get the correct \r\n
> > > sequence and text marches to the right and wraps. This is because the 
> > > TTY is not put into rawmode when QEMU sets it up. 
> > > 
> > > The following patch is a re-diff of a patch applied to Xen's QEMU code.
> > > It uses cfmakeraw() to ensure the TTY is put into rawmode, thus avoiding
> > > the incorrect \r\n translations. It also switches to tcsetattr() on the
> > > slave_fd instead of master_fd - although this is effectively the same on
> > > Linux, only slave_fd works on Solaris. Finally it stops using the 'name'
> > > arg to openpty() which is a security risk because its buffer size is
> > > undefined. Instead it makes use of the ptsname() function.
> > 
> 
> cfmakeraw() is Linux specific, though also available on most BSD
> systems. It is unavailable on Solaris, and probably the same on WIN32.

Win32 is not a problem because the qemu_chr_open_pty() function is already
conditional on 

   #if defined(__linux__) || defined(__sun__)

That, said it is also already  broken on Sun because the call to openpty()
is conditionalized on #if  defined(__linux__) so on Sun it will never
even attempt to open the PTY. 

So this revised patch adds an impl of openty() and cfmakeraw() for Solaris.
I don't have a Solaris system on which to test compile this, but the code
is taken from the Xen fork of QEMU where it has been tested, so it ought 
to do the job.

The updated patch also closes the slave_fd file descriptor since only
the master_fd is needed on the QEMU end.

So with this new patch it should correctly work on Linux and Solaris, 
which are the only platforms qemu_chr_open_pty() is compiled under.
Xen also further extends the conditional around qemu_chr_open_pty() to
allow it to compile on __NetBSD__ and __OpenBSD__. If desired I can
include that, but it is unclear if those needed further code changes or
not - someone might like to just try compiling with this patch on a
BSD variant & see if it works.

    Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel.

Index: vl.c
===================================================================
--- vl.c	(revision 4332)
+++ vl.c	(working copy)
@@ -2269,28 +2269,80 @@
     return chr;
 }
 
+#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(__sun__)
 static CharDriverState *qemu_chr_open_pty(void)
 {
     struct termios tty;
-    char slave_name[1024];
     int master_fd, slave_fd;
 
-#if defined(__linux__)
-    /* Not satisfying */
-    if (openpty(&master_fd, &slave_fd, slave_name, NULL, NULL) < 0) {
+    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) < 0) {
         return NULL;
     }
-#endif
 
-    /* Disabling local echo and line-buffered output */
-    tcgetattr (master_fd, &tty);
-    tty.c_lflag &= ~(ECHO|ICANON|ISIG);
-    tty.c_cc[VMIN] = 1;
-    tty.c_cc[VTIME] = 0;
-    tcsetattr (master_fd, TCSAFLUSH, &tty);
+    /* Set raw attributes on the pty. */
+    cfmakeraw(&tty);
+    tcsetattr(slave_fd, TCSAFLUSH, &tty);
 
-    fprintf(stderr, "char device redirected to %s\n", slave_name);
+    close(slave_fd);
+
+    fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
     return qemu_chr_open_fd(master_fd, master_fd);
 }
 


-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] PATCH: Put Psuedo-TTY in rawmode for char devices

[Samuel Thibault]

Daniel P. Berrange, le Sun 04 May 2008 19:20:17 +0100, a écrit :
> The updated patch also closes the slave_fd file descriptor since only
> the master_fd is needed on the QEMU end.

That will pose problems.  See the corresponding thread on the Xen devel
mailing list `Fix xenconsole's "Could not read tty from store"' from 17
december 2007: « on linux it looks like that makes the master part
stop working: I'm getting EIO on read()s ».

Samuel

Re: [Qemu-devel] PATCH: Put Psuedo-TTY in rawmode for char devices

[Daniel P. Berrange]

On Sun, May 04, 2008 at 07:33:38PM +0100, Samuel Thibault wrote:
> Daniel P. Berrange, le Sun 04 May 2008 19:20:17 +0100, a écrit :
> > The updated patch also closes the slave_fd file descriptor since only
> > the master_fd is needed on the QEMU end.
> 
> That will pose problems.  See the corresponding thread on the Xen devel
> mailing list `Fix xenconsole's "Could not read tty from store"' from 17
> december 2007: « on linux it looks like that makes the master part
> stop working: I'm getting EIO on read()s ».

Opps, thanks for mentioning that - I saw the changeset adding the code to
close(slave_fd) but missed the later changeset removing it. Here's same
patch with that bogus close() removed.

    Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel

Index: vl.c
===================================================================
--- vl.c	(revision 4332)
+++ vl.c	(working copy)
@@ -2269,28 +2269,78 @@
     return chr;
 }
 
+#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(__sun__)
 static CharDriverState *qemu_chr_open_pty(void)
 {
     struct termios tty;
-    char slave_name[1024];
     int master_fd, slave_fd;
 
-#if defined(__linux__)
-    /* Not satisfying */
-    if (openpty(&master_fd, &slave_fd, slave_name, NULL, NULL) < 0) {
+    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) < 0) {
         return NULL;
     }
-#endif
 
-    /* Disabling local echo and line-buffered output */
-    tcgetattr (master_fd, &tty);
-    tty.c_lflag &= ~(ECHO|ICANON|ISIG);
-    tty.c_cc[VMIN] = 1;
-    tty.c_cc[VTIME] = 0;
-    tcsetattr (master_fd, TCSAFLUSH, &tty);
+    /* Set raw attributes on the pty. */
+    cfmakeraw(&tty);
+    tcsetattr(slave_fd, TCSAFLUSH, &tty);
 
-    fprintf(stderr, "char device redirected to %s\n", slave_name);
+    fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
     return qemu_chr_open_fd(master_fd, master_fd);
 }
 


-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|