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

["Daniel P. Berrange" <berrange@redhat.com>]

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