[Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Ian Jackson]

The rest of qemu assumes that IO operations on a CharDriverState do
not block.  Currently there are a couple of cases where such a driver
was set up but the calls to set nonblocking mode were missing:
 * qemu_chr_open_pty
 * qemu_chr_open_pipe
 * qemu_chr_open_stdio

This is fixed by adding two calls to socket_set_nonblock to
qemu_chr_open_fd.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 vl.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 4f31288..c428c7e 100644
--- a/vl.c
+++ b/vl.c
@@ -2090,6 +2090,9 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
     CharDriverState *chr;
     FDCharDriver *s;
 
+    socket_set_nonblock(fd_in);
+    socket_set_nonblock(fd_out);
+
     chr = qemu_mallocz(sizeof(CharDriverState));
     if (!chr)
         return NULL;
-- 
1.4.4.4

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori]

Hi,

Ian Jackson wrote:
> The rest of qemu assumes that IO operations on a CharDriverState do
> not block.  Currently there are a couple of cases where such a driver
> was set up but the calls to set nonblocking mode were missing:
>  * qemu_chr_open_pty
>  * qemu_chr_open_pipe
>  * qemu_chr_open_stdio
>
> This is fixed by adding two calls to socket_set_nonblock to
> qemu_chr_open_fd.
>   

This changes semantics a bit.  Previously, using a pty would guarantee 
that data is always written as qemu_chr_write does not perform any sort 
of buffering.

Now, that data will be silently dropped instead of causing QEMU to 
block.  I don't think it's perfectly clear that one behaviour is clearly 
better than the other.

Regards,

Anthony Liguori

> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  vl.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 4f31288..c428c7e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2090,6 +2090,9 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
>      CharDriverState *chr;
>      FDCharDriver *s;
>  
> +    socket_set_nonblock(fd_in);
> +    socket_set_nonblock(fd_out);
> +
>      chr = qemu_mallocz(sizeof(CharDriverState));
>      if (!chr)
>          return NULL;
>   

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Daniel P. Berrange]

On Tue, Jul 22, 2008 at 08:26:59PM -0500, Anthony Liguori wrote:
> Hi,
> 
> Ian Jackson wrote:
> >The rest of qemu assumes that IO operations on a CharDriverState do
> >not block.  Currently there are a couple of cases where such a driver
> >was set up but the calls to set nonblocking mode were missing:
> > * qemu_chr_open_pty
> > * qemu_chr_open_pipe
> > * qemu_chr_open_stdio
> >
> >This is fixed by adding two calls to socket_set_nonblock to
> >qemu_chr_open_fd.
> >  
> 
> This changes semantics a bit.  Previously, using a pty would guarantee 
> that data is always written as qemu_chr_write does not perform any sort 
> of buffering.
> 
> Now, that data will be silently dropped instead of causing QEMU to 
> block.  I don't think it's perfectly clear that one behaviour is clearly 
> better than the other.

In the case of the PTY backend, the only time I'd expect data to be dropped
is if there was no active slave open. If an application has the PTY open
and is interacting, then I'd want to get all data.  

My use case is that all VMs started will have a serial port configured
with '-serial pty'. The guest OS should not be blocked when writing to
the serial port if no one has the PTY open on the host, but if someone
attaches to it with 'virsh console' then I want to be guartenteed to 
get all data.

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

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Daniel P. Berrange wrote:

> In the case of the PTY backend, the only time I'd expect data to be dropped
> is if there was no active slave open. If an application has the PTY open
> and is interacting, then I'd want to get all data.  

Yep, that would be most useful (and also matches what is done for tcp
for example).  Now the interesting question is:  How can qemu figure (in
a portable way) whenever there is some process listening on the slave side?

> My use case is that all VMs started will have a serial port configured
> with '-serial pty'. The guest OS should not be blocked when writing to
> the serial port if no one has the PTY open on the host, but if someone
> attaches to it with 'virsh console' then I want to be guartenteed to 
> get all data.

/me too.

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Daniel P. Berrange]

On Wed, Jul 23, 2008 at 01:48:14PM +0200, Gerd Hoffmann wrote:
> Daniel P. Berrange wrote:
> 
> > In the case of the PTY backend, the only time I'd expect data to be dropped
> > is if there was no active slave open. If an application has the PTY open
> > and is interacting, then I'd want to get all data.  
> 
> Yep, that would be most useful (and also matches what is done for tcp
> for example).  Now the interesting question is:  How can qemu figure (in
> a portable way) whenever there is some process listening on the slave side?

Well you get SIGHUP when the slave side closes, but the problem is then
detecting the re-connect. If you keep polling on the master PTY you'll just
spin getting SIGHUP all the time. On Linux you can deal with this by using
epoll() in edge-triggered mode instead of poll which is level-triggered.
Unfortunately epoll is Linux specific, so you'd need to have alternate
impls of the guts of the QEMU event loop for every OS :-(

According to epoll(2), on FreeBSD you can use kqueue and on Solaris
you can use /dev/poll. The PTY code isn't supported on Windows so
that wouldn't neccessarily be a show stopper.

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

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Daniel P. Berrange wrote:
> On Wed, Jul 23, 2008 at 01:48:14PM +0200, Gerd Hoffmann wrote:
>> Daniel P. Berrange wrote:
>>
>>> In the case of the PTY backend, the only time I'd expect data to be dropped
>>> is if there was no active slave open. If an application has the PTY open
>>> and is interacting, then I'd want to get all data.  
>> Yep, that would be most useful (and also matches what is done for tcp
>> for example).  Now the interesting question is:  How can qemu figure (in
>> a portable way) whenever there is some process listening on the slave side?
> 
> Well you get SIGHUP when the slave side closes, but the problem is then
> detecting the re-connect.

I don't see a SIGHUP.

Played around a bit with it (on linux) and found:

The write() syscall doesn't show different behavior.  It just queues up
data and either blocks or returns -EAGAIN when the buffer is full.  No
matter whenever someone is connected or not.

The read() syscall blocks or returns -EAGAIN when connected.  It returns
-EIO when unconnected (and flagged ready by select/poll).  So we can
detect disconnects that way.

> If you keep polling on the master PTY you'll just
> spin getting SIGHUP all the time.

Yep.  Well, I've seen it spinning getting -EIO, but thats pretty much
the same issue.

> On Linux you can deal with this by using
> epoll() in edge-triggered mode instead of poll which is level-triggered.
> Unfortunately epoll is Linux specific, so you'd need to have alternate
> impls of the guts of the QEMU event loop for every OS :-(

It is probably easier take the fd out of the polling loop and check now
and then using a timer.  Not perfect, but IMHO better that maintaining
three different poll implementations.

Which means we need our own code for ptys and can't use the generic fd
functions.  I'll go trying cooking up a patch ...

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Daniel P. Berrange]

On Wed, Jul 23, 2008 at 02:52:09PM +0200, Gerd Hoffmann wrote:
> Daniel P. Berrange wrote:
> > On Wed, Jul 23, 2008 at 01:48:14PM +0200, Gerd Hoffmann wrote:
> >> Daniel P. Berrange wrote:
> >>
> >>> In the case of the PTY backend, the only time I'd expect data to be dropped
> >>> is if there was no active slave open. If an application has the PTY open
> >>> and is interacting, then I'd want to get all data.  
> >> Yep, that would be most useful (and also matches what is done for tcp
> >> for example).  Now the interesting question is:  How can qemu figure (in
> >> a portable way) whenever there is some process listening on the slave side?
> > 
> > Well you get SIGHUP when the slave side closes, but the problem is then
> > detecting the re-connect.
> 
> I don't see a SIGHUP.

Of course I meant POLLHUP not SIGHUP ...

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

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

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

  Hi,

> Which means we need our own code for ptys and can't use the generic fd
> functions.  I'll go trying cooking up a patch ...

Comments on this one?

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

[-- Attachment #2: qemu-pty.diff --]
[-- Type: text/x-patch, Size: 4062 bytes --]

diff --git a/vl.c b/vl.c
index 1af6d10..23d92e5 100644
--- a/vl.c
+++ b/vl.c
@@ -2454,21 +2454,153 @@ void cfmakeraw (struct termios *termios_p)
 #endif
 
 #if defined(__linux__) || defined(__sun__)
+
+typedef struct {
+    int fd;
+    int connected;
+    int polling;
+    int read_bytes;
+    int interval;
+    QEMUTimer *timer;
+} PtyCharDriver;
+
+static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+{
+    PtyCharDriver *s = chr->opaque;
+    if (!s->connected)
+	return 0;
+    return unix_write(s->fd, buf, len);
+}
+
+static void pty_chr_state(CharDriverState *chr, int connected)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    if (s->connected == connected)
+	return;
+    if (connected) {
+	fprintf(stderr,"%s: %s connected\n", __FUNCTION__, ptsname(s->fd));
+	s->connected = 1;
+	qemu_chr_reset(chr);
+    } else {
+	fprintf(stderr,"%s: %s disconnected\n", __FUNCTION__, ptsname(s->fd));
+	s->connected = 0;
+	qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + s->interval);
+    }
+}
+
+static int pty_chr_read_poll(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+
+    s->read_bytes = qemu_chr_can_read(chr);
+    return s->read_bytes;
+}
+
+static void pty_chr_read(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+    int size, len;
+    uint8_t buf[1024];
+
+    len = sizeof(buf);
+    if (len > s->read_bytes)
+        len = s->read_bytes;
+    if (len == 0)
+        return;
+    size = read(s->fd, buf, len);
+    if ((size == -1 && EIO == errno) ||
+	(size == 0)) {
+	qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+	s->polling = 0;
+	pty_chr_state(chr, 0);
+        return;
+    }
+    if (size > 0) {
+	pty_chr_state(chr, 1);
+        qemu_chr_read(chr, buf, size);
+    }
+}
+
+static void pty_chr_update_read_handler(CharDriverState *chr)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    qemu_set_fd_handler2(s->fd, pty_chr_read_poll,
+			 pty_chr_read, NULL, chr);
+    s->polling = 1;
+    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + s->interval);
+}
+
+void pty_chr_timer(void *opaque)
+{
+    struct CharDriverState *chr = opaque;    
+    PtyCharDriver *s = chr->opaque;
+
+    if (s->connected) {
+	/* All fine, nothing to do. */
+	return;
+    }
+    if (s->polling) {
+	/* Ran for a while without getting EIO for reads,
+	 * probably someone connected to the slave pty. */
+	pty_chr_state(chr, 1);
+	return;
+    }
+    /* Try reading again ... */
+#if 0
+    fprintf(stderr,"%s: %s checking ...\n", __FUNCTION__, ptsname(s->fd));
+#endif
+    pty_chr_update_read_handler(chr);
+}
+
+static void pty_chr_close(struct CharDriverState *chr)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+    close(s->fd);
+    qemu_free(s);
+}
+
 static CharDriverState *qemu_chr_open_pty(void)
 {
+    CharDriverState *chr;
+    PtyCharDriver *s;
     struct termios tty;
-    int master_fd, slave_fd;
+    int slave_fd;
+    
+    chr = qemu_mallocz(sizeof(CharDriverState));
+    if (!chr)
+        return NULL;
+    s = qemu_mallocz(sizeof(PtyCharDriver));
+    if (!s) {
+        free(chr);
+        return NULL;
+    }
 
-    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) < 0) {
+    if (openpty(&s->fd, &slave_fd, NULL, NULL, NULL) < 0) {
         return NULL;
     }
 
     /* Set raw attributes on the pty. */
     cfmakeraw(&tty);
     tcsetattr(slave_fd, TCSAFLUSH, &tty);
+    close(slave_fd);
+    
+    fprintf(stderr, "char device redirected to %s\n", ptsname(s->fd));
+    
+    chr->opaque = s;
+    chr->chr_write = pty_chr_write;
+    chr->chr_update_read_handler = pty_chr_update_read_handler;
+    chr->chr_close = pty_chr_close;
 
-    fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
-    return qemu_chr_open_fd(master_fd, master_fd);
+    s->interval = 100; /* miliseconds */
+    s->timer = qemu_new_timer(rt_clock, pty_chr_timer, chr);
+    
+    return chr;
 }
 
 static void tty_serial_init(int fd, int speed,

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori]

Gerd Hoffmann wrote:
>   Hi,
>
>   
>> Which means we need our own code for ptys and can't use the generic fd
>> functions.  I'll go trying cooking up a patch ...
>>     
>
> Comments on this one?
>   

Checking every 100ms for every pty device really makes me cringe.

Why is libvirt using ptys in the first place?  Why not use unix 
sockets?  They don't have these problems with state tracking.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>   

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Daniel P. Berrange]

On Wed, Jul 23, 2008 at 10:24:58AM -0500, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
> >  
> >>Which means we need our own code for ptys and can't use the generic fd
> >>functions.  I'll go trying cooking up a patch ...
> >
> >Comments on this one?
> >  
> 
> Checking every 100ms for every pty device really makes me cringe.
> 
> Why is libvirt using ptys in the first place?  Why not use unix 
> sockets?  They don't have these problems with state tracking.

The application using libvirt chooses to use PTYs - we're merely
exposing the capability. The virt-console program for interacting
with serial ports uses PTYs because its a configuration that historically
works with both Xen and KVM. It could equally use UNIX sockets, but it'd
still be desirable for PTYs to work better

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

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Wed, Jul 23, 2008 at 10:24:58AM -0500, Anthony Liguori wrote:
>   
>> Gerd Hoffmann wrote:
>>     
>>>  
>>>       
>>>> Which means we need our own code for ptys and can't use the generic fd
>>>> functions.  I'll go trying cooking up a patch ...
>>>>         
>>> Comments on this one?
>>>  
>>>       
>> Checking every 100ms for every pty device really makes me cringe.
>>
>> Why is libvirt using ptys in the first place?  Why not use unix 
>> sockets?  They don't have these problems with state tracking.
>>     
>
> The application using libvirt chooses to use PTYs - we're merely
> exposing the capability. The virt-console program for interacting
> with serial ports uses PTYs because its a configuration that historically
> works with both Xen and KVM. It could equally use UNIX sockets, but it'd
> still be desirable for PTYs to work better
>   

Okay, I'd strongly suggest migrating to something other than ptys.

Regards,

Anthony Liguori

> Daniel
>   

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Anthony Liguori wrote:
> 
> Okay, I'd strongly suggest migrating to something other than ptys.
> 

Granted.  That still leaves the question open what to do about the
existing pty support.  Try to fix?  Leave it broken?  Drop from qemu?

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori]

Gerd Hoffmann wrote:
> Anthony Liguori wrote:
>   
>> Okay, I'd strongly suggest migrating to something other than ptys.
>>
>>     
>
> Granted.  That still leaves the question open what to do about the
> existing pty support.  Try to fix?  Leave it broken?  Drop from qemu?
>   

I really don't like introducing polling.  I think we should be trying to 
reduce the overall number of wake ups in QEMU, not add more.

I'm inclined to say that we simply declare that reconnecting to a PTY is 
broken/unsupported and strongly encourage the use of something else 
(like unix sockets).  That's always been my understanding FWIW.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>   

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Jamie Lokier]

Anthony Liguori wrote:
> I'm inclined to say that we simply declare that reconnecting to a PTY is 
> broken/unsupported and strongly encourage the use of something else 
> (like unix sockets).  That's always been my understanding FWIW.

As fas as I know, the usual way to use ptys is a program opens the
pty/tty pair, then it forks/execs a process which will use the tty
half.  The child might be the one to open the tty, but it is still
done so the parent knows the tty is open before it tries to use the
pty.

So it's not usual to have a pty open and _wait_ for the tty to open.

That said, a long time ago X11 used to use ptys and did something like
that.  I think it had a main pty, accessed exclusively, for initial
connection - and then allocated a pty/tty pair per individual
connection, freeing up the main one for another client.

To create a pty, and wait without polling for another process to
connect to the tty, this might work:

    1. Open the pty and tty _pair_, in the usual way.
       (openpt, grantpt, unlockpt etc.)
       Make sure the tty doesn't become controlling terminal - TIOCNOTTY.

    2. Keep the tty open, so you poll the pty for I/O as usual.
       Ignore the tty, just keep it open.

    3. Write whatever intro message appears on the pty, same way as
       with a socket.  No need to wait, the message is queued anyway.

    4. When the other process opens the tty, it will read the
       intro message.  When it writes something, QEMU will get read
       availability on the pty and process monitor commands as usual.

    5. If you want to support reconnects, close the tty in QEMU after
       read availabilty on the pty, so you receive EIO/POLLHUP from the pty
       when the other process is finished, or if it calls vhangup().
       Then reopen the tty with TIOCNOTTY, send another intro again.

       This seems like it won't handle a process which connects and
       sends nothing.  You might be able to use TIOCPKT to detect when
       the other process reads any of the intro message, I'm not sure.
       This would be better.

       Actually, if TIOCPKT works maybe you don't need to separately
       open the tty...

I'm not sure if this is complicated by controlling terminals,
sessions, process groups and SIGTTOU/SIGTTINs sent to the other
process, or if it just works out provided the tty descriptor in step 1
is opened with TIOCNOTTY.

-- Jamie

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Jamie Lokier wrote:
> As fas as I know, the usual way to use ptys is a program opens the
> pty/tty pair, then it forks/execs a process which will use the tty
> half.  The child might be the one to open the tty, but it is still
> done so the parent knows the tty is open before it tries to use the
> pty.

Yep, that is what they are designed for (i.e. terminal emulators like
screen or xterm keep the pty and hand over the tty to the shell or
whatever process they are going to run in the virtual terminal).

> To create a pty, and wait without polling for another process to
> connect to the tty, this might work:
> 
>     1. Open the pty and tty _pair_, in the usual way.

[ ... cut ... ]

That would work for the monitor.

The problematic corner case are virtual serial lines though.  If nobody
is connected we don't want to send out data (pretty much like a
unconnected socket), so we don't block forever once the output buffer is
full.  If somebody is listening we want him get all the data (and
eventually block in case the reader is slow).

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Anthony Liguori wrote:
> I really don't like introducing polling.  I think we should be trying to
> reduce the overall number of wake ups in QEMU, not add more.

Right now qemu wakes up quite often anyway, so it doesn't become worse
(bad excuse, I know).

> I'm inclined to say that we simply declare that reconnecting to a PTY is
> broken/unsupported and strongly encourage the use of something else
> (like unix sockets).  That's always been my understanding FWIW.

It's not only reconnects, its also the initial connect.

Right now qemu simply blocks once the pty output buffer is full.  With
nobody reading this effectively means qemu blocks forever and doesn't
responds any more to anything.  It can't stay that way IMO.

I think we have three options here:

  (1) Detect whenever someone is connected.  Drawback: needs polling
      (unless someone comes up with a better patch).
  (2) Run ptys in non-blocking mode.  Drawback: we risk loosing data
      in case the pty reader is slow (also when nobody is connected of
      course, but that is intentional ...).
  (3) Drop pty support.  Drawback: we break existing setups.

Comments?  Other suggestions?

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Daniel P. Berrange]

On Thu, Jul 24, 2008 at 09:54:43AM +0200, Gerd Hoffmann wrote:
> Anthony Liguori wrote:
> > I really don't like introducing polling.  I think we should be trying to
> > reduce the overall number of wake ups in QEMU, not add more.
> 
> Right now qemu wakes up quite often anyway, so it doesn't become worse
> (bad excuse, I know).
> 
> > I'm inclined to say that we simply declare that reconnecting to a PTY is
> > broken/unsupported and strongly encourage the use of something else
> > (like unix sockets).  That's always been my understanding FWIW.
> 
> It's not only reconnects, its also the initial connect.
> 
> Right now qemu simply blocks once the pty output buffer is full.  With
> nobody reading this effectively means qemu blocks forever and doesn't
> responds any more to anything.  It can't stay that way IMO.
> 
> I think we have three options here:
> 
>   (1) Detect whenever someone is connected.  Drawback: needs polling
>       (unless someone comes up with a better patch).
>   (2) Run ptys in non-blocking mode.  Drawback: we risk loosing data
>       in case the pty reader is slow (also when nobody is connected of
>       course, but that is intentional ...).
>   (3) Drop pty support.  Drawback: we break existing setups.

NACK to that last option - I'd rather keep existing blocking behaviour,
than break existing applications using PTYs.

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

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Jamie Lokier]

Gerd Hoffmann wrote:
>   (2) Run ptys in non-blocking mode.  Drawback: we risk loosing data
>       in case the pty reader is slow (also when nobody is connected of
>       course, but that is intentional ...).

(4) Run ptys in non-blocking mode, but use a small output buffer in
    QEMU so nothing is lost.  It's not that hard.

-- Jamie

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Samuel Thibault]

Jamie Lokier, le Thu 24 Jul 2008 10:24:03 +0100, a écrit :
> Gerd Hoffmann wrote:
> >   (2) Run ptys in non-blocking mode.  Drawback: we risk loosing data
> >       in case the pty reader is slow (also when nobody is connected of
> >       course, but that is intentional ...).
> 
> (4) Run ptys in non-blocking mode, but use a small output buffer in
>     QEMU so nothing is lost.  It's not that hard.

Err, there's _already_ a small buffer in the kernel pty layer :)

Samuel

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Samuel Thibault wrote:
> Jamie Lokier, le Thu 24 Jul 2008 10:24:03 +0100, a écrit :
>> Gerd Hoffmann wrote:
>>>   (2) Run ptys in non-blocking mode.  Drawback: we risk loosing data
>>>       in case the pty reader is slow (also when nobody is connected of
>>>       course, but that is intentional ...).
>> (4) Run ptys in non-blocking mode, but use a small output buffer in
>>     QEMU so nothing is lost.  It's not that hard.
> 
> Err, there's _already_ a small buffer in the kernel pty layer :)

Yep, one page (aka 4k on x86) in size.  Of course we can buffer in qemu
too.  That doesn't solve the fundamental issue though (unless we don't
limit the buffer size, which would be insane IMHO).

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>>
>> Comments on this one?
> 
> Checking every 100ms for every pty device really makes me cringe.

Only when unconnected, and the interval can be changed.
And I'm certainly open for better ideas ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori]

Gerd Hoffmann wrote:
> Anthony Liguori wrote:
>   
>> Gerd Hoffmann wrote:
>>     
>>> Comments on this one?
>>>       
>> Checking every 100ms for every pty device really makes me cringe.
>>     
>
> Only when unconnected, and the interval can be changed.
> And I'm certainly open for better ideas ...
>   

I don't have one.  In xenconsoled, once a pty disconnected, it creates a 
new pty specifically because I don't think there's any sane way to 
detect reconnection.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>
>
>   

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Daniel P. Berrange]

On Wed, Jul 23, 2008 at 11:31:42AM -0500, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
> >Anthony Liguori wrote:
> >  
> >>Gerd Hoffmann wrote:
> >>    
> >>>Comments on this one?
> >>>      
> >>Checking every 100ms for every pty device really makes me cringe.
> >>    
> >
> >Only when unconnected, and the interval can be changed.
> >And I'm certainly open for better ideas ...
> >  
> 
> I don't have one.  In xenconsoled, once a pty disconnected, it creates a 
> new pty specifically because I don't think there's any sane way to 
> detect reconnection.

The only guarenteed reliable way I know of is using epoll(), but as I
mentioned before that's Linux specific, so not immediately useful unless
we're willing to drop in an alternate epoll() based main loop for Linux
only, and say other OS have to use PTYs in blocking mode.

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

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Wed, Jul 23, 2008 at 11:31:42AM -0500, Anthony Liguori wrote:
>   
>
> The only guarenteed reliable way I know of is using epoll(), but as I
> mentioned before that's Linux specific, so not immediately useful unless
> we're willing to drop in an alternate epoll() based main loop for Linux
> only, and say other OS have to use PTYs in blocking mode.
>   

So far, this sounds like the best option to me.

Regards,

Anthony Liguori

> Daniel
>   

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Jamie Lokier]

Anthony Liguori wrote:
> >The only guarenteed reliable way I know of is using epoll(), but as I
> >mentioned before that's Linux specific, so not immediately useful unless
> >we're willing to drop in an alternate epoll() based main loop for Linux
> >only, and say other OS have to use PTYs in blocking mode.
> >  
> 
> So far, this sounds like the best option to me.

I'm surprised it works.  You shouldn't be able to get any more
information out of epoll() than poll() - they're supposed to be
exactly equivalent informationally, except for details and performance.

Which suggests to me if it works now, it might get... fixed.

So what are you doing with epoll + pty that works?

Btw, you don't need an epoll main loop.  You can just put the pty into
epoll, and put the _epoll_'s file descriptor into the main loop.
You'll get a ready-to-read when there's a message from epoll.

epoll supports its descriptor being used by other pollers, even
another epoll.

-- Jamie

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Jamie Lokier wrote:
> I'm surprised it works.  You shouldn't be able to get any more
> information out of epoll() than poll() - they're supposed to be
> exactly equivalent informationally, except for details and performance.

I was thinking just the same ...

poll/epoll says the file handle is readable.  It does that in two cases:

  (1) there is data to read
  (2) pty is unconnected and read() wants to give you an -EIO.

And I think you can't keep those two cases apart using only the
information given by poll/epoll ...

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Paul Brook]

On Wednesday 23 July 2008, Gerd Hoffmann wrote:
> Anthony Liguori wrote:
> > Gerd Hoffmann wrote:
> >> Comments on this one?
> >
> > Checking every 100ms for every pty device really makes me cringe.
>
> Only when unconnected, and the interval can be changed.
> And I'm certainly open for better ideas ...

Anything that requires periodic polling is almost certainly wrong.

If it's unconnected then why do we need to poll at all? If we're discarding 
data then we aren't going to retry, so it should be sufficient to check 
whenever we send data.

Paul

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori]

Paul Brook wrote:
> On Wednesday 23 July 2008, Gerd Hoffmann wrote:
>   
>> Anthony Liguori wrote:
>>     
>>> Gerd Hoffmann wrote:
>>>       
>>>> Comments on this one?
>>>>         
>>> Checking every 100ms for every pty device really makes me cringe.
>>>       
>> Only when unconnected, and the interval can be changed.
>> And I'm certainly open for better ideas ...
>>     
>
> Anything that requires periodic polling is almost certainly wrong.
>
> If it's unconnected then why do we need to poll at all? If we're discarding 
> data then we aren't going to retry, so it should be sufficient to check 
> whenever we send data.
>   

There's no way to determine if someone reconnected without trying to do 
a read().  pty's just aren't designed for this sort of thing.

Regards,

Anthony Liguori

> Paul
>
>
>   

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Paul Brook wrote:
> On Wednesday 23 July 2008, Gerd Hoffmann wrote:
>> Anthony Liguori wrote:
>>> Gerd Hoffmann wrote:
>>>> Comments on this one?
>>> Checking every 100ms for every pty device really makes me cringe.
>> Only when unconnected, and the interval can be changed.
>> And I'm certainly open for better ideas ...
> 
> Anything that requires periodic polling is almost certainly wrong.
> 
> If it's unconnected then why do we need to poll at all? If we're discarding 
> data then we aren't going to retry, so it should be sufficient to check 
> whenever we send data.

Fundamental problem is there is no easy way to figure whenever we are
connected or not.  Well, detecting the "connected -> disconnected"
transition is easy, as read() starts giving us -EIO then.  The
problematic case is the "(initial state | disconnected) ->
(re-)connected" transition.  We have to try read() now and then to check
whenever we still get -EIO or not.

Doing the read() check in the write path is an good idea.  Alone it
still doesn't solve the problem because we would depend on the guest
writing stuff to the serial line to detect a (re-)connect then.  But it
 makes large poll intervalls suck less.  I'll go send an updated patch
later today.

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Jamie Lokier]

Gerd Hoffmann wrote:
> Fundamental problem is there is no easy way to figure whenever we are
> connected or not.  Well, detecting the "connected -> disconnected"
> transition is easy, as read() starts giving us -EIO then.  The
> problematic case is the "(initial state | disconnected) ->
> (re-)connected" transition.  We have to try read() now and then to check
> whenever we still get -EIO or not.

Btw, I've just tested.  In the initial state, the tty side never
opened, read() blocks and poll/select report that it's not ready for
read.

So the initial state is no problem, it doesn't need polling.

Only disconnected -> reconnected is a problem.

-- Jamie

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Jamie Lokier wrote:
> Gerd Hoffmann wrote:
>> Fundamental problem is there is no easy way to figure whenever we are
>> connected or not.  Well, detecting the "connected -> disconnected"
>> transition is easy, as read() starts giving us -EIO then.  The
>> problematic case is the "(initial state | disconnected) ->
>> (re-)connected" transition.  We have to try read() now and then to check
>> whenever we still get -EIO or not.
> 
> Btw, I've just tested.  In the initial state, the tty side never
> opened, read() blocks and poll/select report that it's not ready for
> read.

You still don't know whenever someone is connected or not.  And thus you
still don't know whenever the stuff you write to the tty is read out by
someone or you risk to block when the kernel buffer is full.

Also note that openpty() which is used by qemu gives you an open file
handle for the tty side, so there is no "tty never opened" state.

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Jamie Lokier]

Gerd Hoffmann wrote:
> > Btw, I've just tested.  In the initial state, the tty side never
> > opened, read() blocks and poll/select report that it's not ready for
> > read.
> 
> You still don't know whenever someone is connected or not.
> And thus you still don't know whenever the stuff you write to the
> tty is read out by someone or you risk to block when the kernel
> buffer is full.

That's no different from someone connected to the tty and not reading.

Is the problem that you want qemu to block when they do that?

> Also note that openpty() which is used by qemu gives you an open file
> handle for the tty side, so there is no "tty never opened" state.

Sure, but it's easy enough to use openpt(), grantpt() and unlockpt()
instead, as described in the Glibc manual.

-- Jamie

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Jamie Lokier wrote:
> Gerd Hoffmann wrote:
>>> Btw, I've just tested.  In the initial state, the tty side never
>>> opened, read() blocks and poll/select report that it's not ready for
>>> read.
>> You still don't know whenever someone is connected or not.
>> And thus you still don't know whenever the stuff you write to the
>> tty is read out by someone or you risk to block when the kernel
>> buffer is full.
> 
> That's no different from someone connected to the tty and not reading.

Yes.

> Is the problem that you want qemu to block when they do that?

I want ptys behave as close as possible to tcp/unix sockets:

 * when nobody is connected, then don't send data.  Because we will
   block forever once the kernel buffer is full, which is bad.
 * when someone is connected, we want him get all data.  If the reader
   is too slow to keep up, that means we will block now and then, yes.
   Sockets do that too.

Additionally we can also try to drive the ptys / sockets / whatever
filehandles in non-blocking mode and try propagating the state back to
the guest so it stops writing.  That is a different issue, although
related.  IIRC the xen guys have some bits for that ...

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[David Woodhouse]

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

(very old thread)

On Mon, 2008-07-28 at 14:20 +0200, Gerd Hoffmann wrote:
> Jamie Lokier wrote:
> > Is the problem that you want qemu to block when they do that?
> I want ptys behave as close as possible to tcp/unix sockets:
> 
>  * when nobody is connected, then don't send data.  Because we will
>    block forever once the kernel buffer is full, which is bad.
>  * when someone is connected, we want him get all data.  If the reader
>    is too slow to keep up, that means we will block now and then, yes.
>    Sockets do that too.

Hm? Sockets operate in O_NONBLOCK mode, don't they? 

If the other end stops consuming data and the kernel buffers fill up,
the serial driver ignores the resulting -EAGAIN and drops the byte.

And although it's "lossy", it's sane behaviour. It's exactly what you
get on a real RS232 serial line too; if the other end doesn't keep up
with reading the data out of its end, *it* is going to miss bytes.
That's always been the case, and I don't think we need to worry about
it.

We *certainly* shouldn't be blocking the whole guest CPU in the outb
instruction, waiting for ever for the other end to consume data. Which
is what we seem to do at the moment for PTYs.

> Additionally we can also try to drive the ptys / sockets / whatever
> filehandles in non-blocking mode and try propagating the state back to
> the guest so it stops writing.  That is a different issue, although
> related.  IIRC the xen guys have some bits for that ...

We *could*, I suppose, although as noted I'm not sure it's necessary. A
relatively easy answer might be for the serial driver to spot the
-EAGAIN and keep the byte in the transmit FIFO/register, and refrain
from reasserting THRE/TEMT until it's actually managed to send the
byte.

If we really wanted to support fully lossless serial communication, a
better option might be to implement the hardware flow control lines,
and assert CTS only when the (pty/socket/etc.) file descriptor is
writable. Then *if* the guest chooses to use hardware flow control,
that ought to work.

But I still don't think it's necessary. Can't we just make the PTY file
descriptor non-blocking, and then perhaps the monitoring logic added in
commit 279e694bc ("Attempt to detect unconnected ptys (Gerd Hoffman)")
can get simpler because it only has to poll an active device for
connectivity in the case where the write() fails.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[Qemu-devel] Re: [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori]

Gerd Hoffmann wrote:
>   Hi,
> 
>> Which means we need our own code for ptys and can't use the generic fd
>> functions.  I'll go trying cooking up a patch ...
> 
> Comments on this one?

If this is the best we can do (and I certainly hope we can do better), 
then I would at least like to see the interval bumped up to a second. 
I'd also like to see the qemu_mod_timer API extended to accept timeouts 
in milliseconds or seconds such that the later can be coalesced into a 
single wake-up.

An alternative implementation would be to use one timer for all PTY 
devices but I think adding a qemu_mod_timer_sec() is a more useful 
general thing to do.

Regards,

Anthony Liguori

> cheers,
>   Gerd
> 
> 

[Qemu-devel] Re: [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

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

Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>>   Hi,
>>
>>> Which means we need our own code for ptys and can't use the generic fd
>>> functions.  I'll go trying cooking up a patch ...
>>
>> Comments on this one?
> 
> If this is the best we can do (and I certainly hope we can do better),
> then I would at least like to see the interval bumped up to a second.

Done.  Also implemented Pauls idea to check the status on guest writes.
 Added a bunch of comments for the state tracking.

Comments?

  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

[-- Attachment #2: 0003-Attempt-to-detect-unconnected-ptys.patch --]
[-- Type: text/x-patch, Size: 6107 bytes --]

>From 076b5cff55b57a64e5f8daf32186d924945f78b4 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 23 Jul 2008 16:35:40 +0200
Subject: [PATCH 03/21] Attempt to detect unconnected ptys.

This patch moves the pty char device imlementation away from the generic
filehandle code.  It tries to detect as good as possible whenever there
is someone connected to the slave pty device and only send data down the
road in case someone is listening.  Unfortunaly we have to poll via
timer once in a while to check the status because we have to use read()
on the master pty to figure the status (returns -EIO when unconnected).

Poll intervall for an idle guest is one second, when the guest sends
data to the virtual device linked to the pty we check more frequently.

The point for all of this is to avoid qemu blocking and not responding
any more.  Writing to the master pty handle succeeds even when nobody is
connected to (and reading from) to the slave end of the pty.  The kernel
just bufferes the writes.  And as soon as the kernel buffer is full the
write() call blocks forever ...

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vl.c |  158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 154 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 23b5243..faf7b95 100644
--- a/vl.c
+++ b/vl.c
@@ -2474,21 +2474,171 @@ void cfmakeraw (struct termios *termios_p)
 #endif
 
 #if defined(__linux__) || defined(__sun__)
+
+typedef struct {
+    int fd;
+    int connected;
+    int polling;
+    int read_bytes;
+    QEMUTimer *timer;
+} PtyCharDriver;
+
+static void pty_chr_update_read_handler(CharDriverState *chr);
+static void pty_chr_state(CharDriverState *chr, int connected);
+
+static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    if (!s->connected) {
+	/* guest sends data, check for (re-)connect */
+	pty_chr_update_read_handler(chr);
+	return 0;
+    }
+    return unix_write(s->fd, buf, len);
+}
+
+static int pty_chr_read_poll(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+
+    s->read_bytes = qemu_chr_can_read(chr);
+    return s->read_bytes;
+}
+
+static void pty_chr_read(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+    int size, len;
+    uint8_t buf[1024];
+
+    len = sizeof(buf);
+    if (len > s->read_bytes)
+        len = s->read_bytes;
+    if (len == 0)
+        return;
+    size = read(s->fd, buf, len);
+    if ((size == -1 && EIO == errno) ||
+	(size == 0)) {
+	qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+	s->polling = 0;
+	pty_chr_state(chr, 0);
+        return;
+    }
+    if (size > 0) {
+	pty_chr_state(chr, 1);
+        qemu_chr_read(chr, buf, size);
+    }
+}
+
+static void pty_chr_update_read_handler(CharDriverState *chr)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    qemu_set_fd_handler2(s->fd, pty_chr_read_poll,
+			 pty_chr_read, NULL, chr);
+    s->polling = 1;
+    /*
+     * Short timeout here: just need wait long enougth that qemu makes
+     * it through the poll loop once.  When reconnected we want a
+     * short timeout so we notice it almost instantly.  Otherwise
+     * read() gives us -EIO instantly, making pty_chr_state() reset the
+     * timeout to the normal (much longer) poll interval before the
+     * timer triggers.
+     */
+    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 10);
+}
+
+static void pty_chr_state(CharDriverState *chr, int connected)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    if (!connected) {
+	if (s->connected) {
+	    fprintf(stderr,"%s: %s disconnected\n", __FUNCTION__, ptsname(s->fd));
+	    s->connected = 0;
+	}
+	/* (re-)connect poll interval for idle guests: once per second.
+	 * We check more frequently in case the guests sends data to
+	 * the virtual device linked to our pty. */
+	qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 1000);
+    } else {
+	if (!s->connected) {
+	    fprintf(stderr,"%s: %s connected\n", __FUNCTION__, ptsname(s->fd));
+	    s->connected = 1;
+	    qemu_chr_reset(chr);
+	}
+    }
+}
+
+void pty_chr_timer(void *opaque)
+{
+    struct CharDriverState *chr = opaque;    
+    PtyCharDriver *s = chr->opaque;
+
+    if (s->connected) {
+	/* All fine, nothing to do. */
+	return;
+    }
+    if (s->polling) {
+	/* Ran for a while without getting EIO for reads,
+	 * probably someone connected to the slave pty. */
+	pty_chr_state(chr, 1);
+	return;
+    }
+    /* Try reading again ... */
+#if 0
+    fprintf(stderr,"%s: %s checking ...\n", __FUNCTION__, ptsname(s->fd));
+#endif
+    pty_chr_update_read_handler(chr);
+}
+
+static void pty_chr_close(struct CharDriverState *chr)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+    close(s->fd);
+    qemu_free(s);
+}
+
 static CharDriverState *qemu_chr_open_pty(void)
 {
+    CharDriverState *chr;
+    PtyCharDriver *s;
     struct termios tty;
-    int master_fd, slave_fd;
+    int slave_fd;
+    
+    chr = qemu_mallocz(sizeof(CharDriverState));
+    if (!chr)
+        return NULL;
+    s = qemu_mallocz(sizeof(PtyCharDriver));
+    if (!s) {
+        free(chr);
+        return NULL;
+    }
 
-    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) < 0) {
+    if (openpty(&s->fd, &slave_fd, NULL, NULL, NULL) < 0) {
         return NULL;
     }
 
     /* Set raw attributes on the pty. */
     cfmakeraw(&tty);
     tcsetattr(slave_fd, TCSAFLUSH, &tty);
+    close(slave_fd);
+    
+    fprintf(stderr, "char device redirected to %s\n", ptsname(s->fd));
+    
+    chr->opaque = s;
+    chr->chr_write = pty_chr_write;
+    chr->chr_update_read_handler = pty_chr_update_read_handler;
+    chr->chr_close = pty_chr_close;
 
-    fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
-    return qemu_chr_open_fd(master_fd, master_fd);
+    s->timer = qemu_new_timer(rt_clock, pty_chr_timer, chr);
+    
+    return chr;
 }
 
 static void tty_serial_init(int fd, int speed,
-- 
1.5.4.1

[Qemu-devel] Re: [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori]

Gerd Hoffmann wrote:
> From 076b5cff55b57a64e5f8daf32186d924945f78b4 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Wed, 23 Jul 2008 16:35:40 +0200
> Subject: [PATCH 03/21] Attempt to detect unconnected ptys.
>
> This patch moves the pty char device imlementation away from the generic
> filehandle code.  It tries to detect as good as possible whenever there
> is someone connected to the slave pty device and only send data down the
> road in case someone is listening.  Unfortunaly we have to poll via
> timer once in a while to check the status because we have to use read()
> on the master pty to figure the status (returns -EIO when unconnected).
>
> Poll intervall for an idle guest is one second, when the guest sends
> data to the virtual device linked to the pty we check more frequently.
>
> The point for all of this is to avoid qemu blocking and not responding
> any more.  Writing to the master pty handle succeeds even when nobody is
> connected to (and reading from) to the slave end of the pty.  The kernel
> just bufferes the writes.  And as soon as the kernel buffer is full the
> write() call blocks forever ...
>   

You've got whitespace damage.  You're mixing tabs and spaces.  Please 
only use spaces.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  vl.c |  158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 154 insertions(+), 4 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 23b5243..faf7b95 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2474,21 +2474,171 @@ void cfmakeraw (struct termios *termios_p)
>  #endif
>  
>  #if defined(__linux__) || defined(__sun__)
> +
> +typedef struct {
> +    int fd;
> +    int connected;
> +    int polling;
> +    int read_bytes;
> +    QEMUTimer *timer;
> +} PtyCharDriver;
> +
> +static void pty_chr_update_read_handler(CharDriverState *chr);
> +static void pty_chr_state(CharDriverState *chr, int connected);
> +
> +static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> +{
> +    PtyCharDriver *s = chr->opaque;
> +
> +    if (!s->connected) {
> +	/* guest sends data, check for (re-)connect */
> +	pty_chr_update_read_handler(chr);
> +	return 0;
> +    }
> +    return unix_write(s->fd, buf, len);
> +}
> +
> +static int pty_chr_read_poll(void *opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    PtyCharDriver *s = chr->opaque;
> +
> +    s->read_bytes = qemu_chr_can_read(chr);
> +    return s->read_bytes;
> +}
> +
> +static void pty_chr_read(void *opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    PtyCharDriver *s = chr->opaque;
> +    int size, len;
> +    uint8_t buf[1024];
> +
> +    len = sizeof(buf);
> +    if (len > s->read_bytes)
> +        len = s->read_bytes;
> +    if (len == 0)
> +        return;
> +    size = read(s->fd, buf, len);
> +    if ((size == -1 && EIO == errno) ||
> +	(size == 0)) {
>   

errno == EIO

> +	qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> +	s->polling = 0;
> +	pty_chr_state(chr, 0);
> +        return;
> +    }
> +    if (size > 0) {
> +	pty_chr_state(chr, 1);
> +        qemu_chr_read(chr, buf, size);
> +    }
> +}
> +
> +static void pty_chr_update_read_handler(CharDriverState *chr)
> +{
> +    PtyCharDriver *s = chr->opaque;
> +
> +    qemu_set_fd_handler2(s->fd, pty_chr_read_poll,
> +			 pty_chr_read, NULL, chr);
> +    s->polling = 1;
> +    /*
> +     * Short timeout here: just need wait long enougth that qemu makes
> +     * it through the poll loop once.  When reconnected we want a
> +     * short timeout so we notice it almost instantly.  Otherwise
> +     * read() gives us -EIO instantly, making pty_chr_state() reset the
> +     * timeout to the normal (much longer) poll interval before the
> +     * timer triggers.
> +     */
> +    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 10);
>   

Perhaps what you really want here is a bottom half?

> +}
> +
> +static void pty_chr_state(CharDriverState *chr, int connected)
> +{
> +    PtyCharDriver *s = chr->opaque;
> +
> +    if (!connected) {
> +	if (s->connected) {
> +	    fprintf(stderr,"%s: %s disconnected\n", __FUNCTION__, ptsname(s->fd));
>   

Obviously this debugging has to go.

> +	    s->connected = 0;
> +	}
> +	/* (re-)connect poll interval for idle guests: once per second.
> +	 * We check more frequently in case the guests sends data to
> +	 * the virtual device linked to our pty. */
> +	qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 1000);
> +    } else {
> +	if (!s->connected) {
> +	    fprintf(stderr,"%s: %s connected\n", __FUNCTION__, ptsname(s->fd));
> +	    s->connected = 1;
> +	    qemu_chr_reset(chr);
> +	}
> +    }
> +}
> +
> +void pty_chr_timer(void *opaque)
> +{
> +    struct CharDriverState *chr = opaque;    
> +    PtyCharDriver *s = chr->opaque;
> +
> +    if (s->connected) {
> +	/* All fine, nothing to do. */
> +	return;
> +    }
> +    if (s->polling) {
> +	/* Ran for a while without getting EIO for reads,
> +	 * probably someone connected to the slave pty. */
> +	pty_chr_state(chr, 1);
> +	return;
> +    }
> +    /* Try reading again ... */
> +#if 0
> +    fprintf(stderr,"%s: %s checking ...\n", __FUNCTION__, ptsname(s->fd));
> +#endif
> +    pty_chr_update_read_handler(chr);
> +}
> +
> +static void pty_chr_close(struct CharDriverState *chr)
> +{
> +    PtyCharDriver *s = chr->opaque;
> +
> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> +    close(s->fd);
> +    qemu_free(s);
> +}
> +
>  static CharDriverState *qemu_chr_open_pty(void)
>  {
> +    CharDriverState *chr;
> +    PtyCharDriver *s;
>      struct termios tty;
> -    int master_fd, slave_fd;
> +    int slave_fd;
> +    
> +    chr = qemu_mallocz(sizeof(CharDriverState));
> +    if (!chr)
> +        return NULL;
> +    s = qemu_mallocz(sizeof(PtyCharDriver));
> +    if (!s) {
> +        free(chr);
>   

qemu_free()

Regards,

Anthony Liguori

> +        return NULL;
> +    }
>  
> -    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) < 0) {
> +    if (openpty(&s->fd, &slave_fd, NULL, NULL, NULL) < 0) {
>          return NULL;
>      }
>  
>      /* Set raw attributes on the pty. */
>      cfmakeraw(&tty);
>      tcsetattr(slave_fd, TCSAFLUSH, &tty);
> +    close(slave_fd);
> +    
> +    fprintf(stderr, "char device redirected to %s\n", ptsname(s->fd));
> +    
> +    chr->opaque = s;
> +    chr->chr_write = pty_chr_write;
> +    chr->chr_update_read_handler = pty_chr_update_read_handler;
> +    chr->chr_close = pty_chr_close;
>  
> -    fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
> -    return qemu_chr_open_fd(master_fd, master_fd);
> +    s->timer = qemu_new_timer(rt_clock, pty_chr_timer, chr);
> +    
> +    return chr;
>  }
>  
>  static void tty_serial_init(int fd, int speed,
> -- 1.5.4.1

[Qemu-devel] Re: [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

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

> You've got whitespace damage.  You're mixing tabs and spaces.  Please
> only use spaces.

Fixed.

>> +    if ((size == -1 && EIO == errno) ||
>> +    (size == 0)) {
>>   
> 
> errno == EIO

Fixed.

>> +    /*
>> +     * Short timeout here: just need wait long enougth that qemu makes
>> +     * it through the poll loop once.  When reconnected we want a
>> +     * short timeout so we notice it almost instantly.  Otherwise
>> +     * read() gives us -EIO instantly, making pty_chr_state() reset the
>> +     * timeout to the normal (much longer) poll interval before the
>> +     * timer triggers.
>> +     */
>> +    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 10);
>>   
> 
> Perhaps what you really want here is a bottom half?

Looked promising, tried, doesn't work.  bh is called *before* the pty
file handle is checked at least once for being readable.

>> +    if (s->connected) {
>> +        fprintf(stderr,"%s: %s disconnected\n", __FUNCTION__,
>> ptsname(s->fd));
>>   
> 
> Obviously this debugging has to go.

Dropped.

>> +    /* Try reading again ... */
>> +#if 0
>> +    fprintf(stderr,"%s: %s checking ...\n", __FUNCTION__,
>> ptsname(s->fd));
>> +#endif

... this too while being at it ;)

>> +    if (!s) {
>> +        free(chr);
>>   
> 
> qemu_free()

Fixed.

New patch attached.

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

[-- Attachment #2: 0003-Attempt-to-detect-unconnected-ptys.patch --]
[-- Type: text/x-patch, Size: 5938 bytes --]

>From 4362b66782ac68d8e1efb04fea1befeb20587677 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 23 Jul 2008 16:35:40 +0200
Subject: [PATCH 03/23] Attempt to detect unconnected ptys.

This patch moves the pty char device imlementation away from the generic
filehandle code.  It tries to detect as good as possible whenever there
is someone connected to the slave pty device and only send data down the
road in case someone is listening.  Unfortunaly we have to poll via
timer once in a while to check the status because we have to use read()
on the master pty to figure the status (returns -EIO when unconnected).

Poll intervall for an idle guest is one second, when the guest sends
data to the virtual device linked to the pty we check more frequently.

The point for all of this is to avoid qemu blocking and not responding
any more.  Writing to the master pty handle succeeds even when nobody is
connected to (and reading from) to the slave end of the pty.  The kernel
just bufferes the writes.  And as soon as the kernel buffer is full the
write() call blocks forever ...

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vl.c |  149 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 145 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 23b5243..8801615 100644
--- a/vl.c
+++ b/vl.c
@@ -2474,21 +2474,162 @@ void cfmakeraw (struct termios *termios_p)
 #endif
 
 #if defined(__linux__) || defined(__sun__)
+
+typedef struct {
+    int fd;
+    int connected;
+    int polling;
+    int read_bytes;
+    QEMUTimer *timer;
+} PtyCharDriver;
+
+static void pty_chr_update_read_handler(CharDriverState *chr);
+static void pty_chr_state(CharDriverState *chr, int connected);
+
+static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    if (!s->connected) {
+        /* guest sends data, check for (re-)connect */
+        pty_chr_update_read_handler(chr);
+        return 0;
+    }
+    return unix_write(s->fd, buf, len);
+}
+
+static int pty_chr_read_poll(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+
+    s->read_bytes = qemu_chr_can_read(chr);
+    return s->read_bytes;
+}
+
+static void pty_chr_read(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+    int size, len;
+    uint8_t buf[1024];
+
+    len = sizeof(buf);
+    if (len > s->read_bytes)
+        len = s->read_bytes;
+    if (len == 0)
+        return;
+    size = read(s->fd, buf, len);
+    if ((size == -1 && errno == EIO) ||
+        (size == 0)) {
+        pty_chr_state(chr, 0);
+        return;
+    }
+    if (size > 0) {
+        pty_chr_state(chr, 1);
+        qemu_chr_read(chr, buf, size);
+    }
+}
+
+static void pty_chr_update_read_handler(CharDriverState *chr)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    qemu_set_fd_handler2(s->fd, pty_chr_read_poll,
+                         pty_chr_read, NULL, chr);
+    s->polling = 1;
+    /*
+     * Short timeout here: just need wait long enougth that qemu makes
+     * it through the poll loop once.  When reconnected we want a
+     * short timeout so we notice it almost instantly.  Otherwise
+     * read() gives us -EIO instantly, making pty_chr_state() reset the
+     * timeout to the normal (much longer) poll interval before the
+     * timer triggers.
+     */
+    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 10);
+}
+
+static void pty_chr_state(CharDriverState *chr, int connected)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    if (!connected) {
+        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+        s->connected = 0;
+        s->polling = 0;
+        /* (re-)connect poll interval for idle guests: once per second.
+         * We check more frequently in case the guests sends data to
+         * the virtual device linked to our pty. */
+        qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 1000);
+    } else {
+        if (!s->connected)
+            qemu_chr_reset(chr);
+        s->connected = 1;
+    }
+}
+
+void pty_chr_timer(void *opaque)
+{
+    struct CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+
+    if (s->connected)
+        return;
+    if (s->polling) {
+        /* If we arrive here without polling being cleared due
+         * read returning -EIO, then we are (re-)connected */
+        pty_chr_state(chr, 1);
+        return;
+    }
+
+    /* Next poll ... */
+    pty_chr_update_read_handler(chr);
+}
+
+static void pty_chr_close(struct CharDriverState *chr)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+    close(s->fd);
+    qemu_free(s);
+}
+
 static CharDriverState *qemu_chr_open_pty(void)
 {
+    CharDriverState *chr;
+    PtyCharDriver *s;
     struct termios tty;
-    int master_fd, slave_fd;
+    int slave_fd;
 
-    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) < 0) {
+    chr = qemu_mallocz(sizeof(CharDriverState));
+    if (!chr)
+        return NULL;
+    s = qemu_mallocz(sizeof(PtyCharDriver));
+    if (!s) {
+        qemu_free(chr);
+        return NULL;
+    }
+
+    if (openpty(&s->fd, &slave_fd, NULL, NULL, NULL) < 0) {
         return NULL;
     }
 
     /* Set raw attributes on the pty. */
     cfmakeraw(&tty);
     tcsetattr(slave_fd, TCSAFLUSH, &tty);
+    close(slave_fd);
+
+    fprintf(stderr, "char device redirected to %s\n", ptsname(s->fd));
 
-    fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
-    return qemu_chr_open_fd(master_fd, master_fd);
+    chr->opaque = s;
+    chr->chr_write = pty_chr_write;
+    chr->chr_update_read_handler = pty_chr_update_read_handler;
+    chr->chr_close = pty_chr_close;
+
+    s->timer = qemu_new_timer(rt_clock, pty_chr_timer, chr);
+
+    return chr;
 }
 
 static void tty_serial_init(int fd, int speed,
-- 
1.5.4.1

[Qemu-devel] Re: [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori]

Gerd Hoffmann wrote:
> Fixed.
>
> New patch attached.
>   

Applied.   Thanks.

In the future, please post new patches as top-level posts.  It makes it 
less likely to miss them.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>   

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Kevin Wolf]

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

Anthony Liguori schrieb:
> This changes semantics a bit.  Previously, using a pty would guarantee
> that data is always written as qemu_chr_write does not perform any sort
> of buffering.
> 
> Now, that data will be silently dropped instead of causing QEMU to
> block.  I don't think it's perfectly clear that one behaviour is clearly
> better than the other.

Then we need both.

Add an option to specify non-blocking mode for stdio, pty and pipe in
qemu_chr_open.  Blocking mode is used by default (maintaining current
semantics), non-blocking mode if the name is prefixed with nonblock:

Signed-off-by: Kevin Wolf <kwolf@suse.de>

[-- Attachment #2: nonblock.patch --]
[-- Type: text/x-patch, Size: 3980 bytes --]

Index: vl.c
===================================================================
--- vl.c.orig
+++ vl.c
@@ -2227,11 +2227,16 @@ static void fd_chr_close(struct CharDriv
 }
 
 /* open a character device to a unix fd */
-static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
+static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out, int block)
 {
     CharDriverState *chr;
     FDCharDriver *s;
 
+    if (!block) {
+        socket_set_nonblock(fd_in);
+        socket_set_nonblock(fd_out);
+    }
+
     chr = qemu_mallocz(sizeof(CharDriverState));
     if (!chr)
         return NULL;
@@ -2259,10 +2264,10 @@ static CharDriverState *qemu_chr_open_fi
     TFR(fd_out = open(file_out, O_WRONLY | O_TRUNC | O_CREAT | O_BINARY, 0666));
     if (fd_out < 0)
         return NULL;
-    return qemu_chr_open_fd(-1, fd_out);
+    return qemu_chr_open_fd(-1, fd_out, 0);
 }
 
-static CharDriverState *qemu_chr_open_pipe(const char *filename)
+static CharDriverState *qemu_chr_open_pipe(const char *filename, int block)
 {
     int fd_in, fd_out;
     char filename_in[256], filename_out[256];
@@ -2280,7 +2285,7 @@ static CharDriverState *qemu_chr_open_pi
         if (fd_in < 0)
             return NULL;
     }
-    return qemu_chr_open_fd(fd_in, fd_out);
+    return qemu_chr_open_fd(fd_in, fd_out, block);
 }
 
 
@@ -2376,13 +2381,13 @@ static void qemu_chr_close_stdio(struct
     fd_chr_close(chr);
 }
 
-static CharDriverState *qemu_chr_open_stdio(void)
+static CharDriverState *qemu_chr_open_stdio(int block)
 {
     CharDriverState *chr;
 
     if (stdio_nb_clients >= STDIO_MAX_CLIENTS)
         return NULL;
-    chr = qemu_chr_open_fd(0, 1);
+    chr = qemu_chr_open_fd(0, 1, block);
     chr->chr_close = qemu_chr_close_stdio;
     qemu_set_fd_handler2(0, stdio_read_poll, stdio_read, NULL, chr);
     stdio_nb_clients++;
@@ -2449,7 +2454,7 @@ void cfmakeraw (struct termios *termios_
 #endif
 
 #if defined(__linux__) || defined(__sun__)
-static CharDriverState *qemu_chr_open_pty(void)
+static CharDriverState *qemu_chr_open_pty(int block)
 {
     struct termios tty;
     int master_fd, slave_fd;
@@ -2463,7 +2468,7 @@ static CharDriverState *qemu_chr_open_pt
     tcsetattr(slave_fd, TCSAFLUSH, &tty);
 
     fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
-    return qemu_chr_open_fd(master_fd, master_fd);
+    return qemu_chr_open_fd(master_fd, master_fd, block);
 }
 
 static void tty_serial_init(int fd, int speed,
@@ -2578,7 +2583,7 @@ static CharDriverState *qemu_chr_open_tt
 
     TFR(fd = open(filename, O_RDWR | O_NONBLOCK));
     tty_serial_init(fd, 115200, 'N', 8, 1);
-    chr = qemu_chr_open_fd(fd, fd);
+    chr = qemu_chr_open_fd(fd, fd, 0);
     if (!chr) {
         close(fd);
         return NULL;
@@ -2588,7 +2593,7 @@ static CharDriverState *qemu_chr_open_tt
     return chr;
 }
 #else  /* ! __linux__ && ! __sun__ */
-static CharDriverState *qemu_chr_open_pty(void)
+static CharDriverState *qemu_chr_open_pty(int block)
 {
     return NULL;
 }
@@ -3582,6 +3587,10 @@ static CharDriverState *qemu_chr_open_tc
 CharDriverState *qemu_chr_open(const char *filename)
 {
     const char *p;
+    int block = 1;
+
+    if (strstart(filename, "nonblock:", &filename))
+        block = 0;
 
     if (!strcmp(filename, "vc")) {
         return text_console_init(&display_state, 0);
@@ -3615,11 +3624,11 @@ CharDriverState *qemu_chr_open(const cha
     } else if (strstart(filename, "file:", &p)) {
         return qemu_chr_open_file_out(p);
     } else if (strstart(filename, "pipe:", &p)) {
-        return qemu_chr_open_pipe(p);
+        return qemu_chr_open_pipe(p, block);
     } else if (!strcmp(filename, "pty")) {
-        return qemu_chr_open_pty();
+        return qemu_chr_open_pty(block);
     } else if (!strcmp(filename, "stdio")) {
-        return qemu_chr_open_stdio();
+        return qemu_chr_open_stdio(block);
     } else
 #if defined(__linux__)
     if (strstart(filename, "/dev/parport", NULL)) {

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd."):
> This changes semantics a bit.  Previously, using a pty would guarantee 
> that data is always written as qemu_chr_write does not perform any sort 
> of buffering.

That depends on what the caller (that is, the internal device model
which is using the char driver) does.

In the case of the serial driver in our tree, the emulated serial port
does not empty its simulated transmit shift register until the data
has been accepted by the char driver - ie, actually delivered to
whereever it is going.

That is, the flow control is communicated back to the guest - perhaps
not in the most optimal way, since perhaps it would be better to
simulate modem flow control lines rather than playing games with
making virtual time run slowly.  But no data will be lost unless the
guest chooses to throw it away - and the guest can get on with the
rest of its life in the meantime.

I'm afraid that this change (which is part of a general improvement to
the flow control handling in hw/serial.c) hasn't made it upstream yet.

But regardless, I think having the whole qemu process block while
writing to a `slow' device like a pty (or a network connection for
that matter) is generally wrong.

I haven't searched for other callers, but if they don't offer a way to
prevent the guest from attempting to write too much then I think that
is a bug in that driver.

Kevin Wolf writes ("Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for	qemu_chr_open_fd."):
> +    if (strstart(filename, "nonblock:", &filename))
> +        block = 0;

This is definitely wrong, because the need for (non)blocking behaviour
is determined statically by the code which is using the driver.

Perhaps there should be two writing interfaces, qemu_chr_write and
qemu_chr_write_(non)block.

qemu_chr_write_block_fd would have to contain a select() or poll()
loop since the fd needs to be in nonblocking mode for the reads to
work correctly without races.

Ian.

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Kevin Wolf]

Ian Jackson schrieb:
> Kevin Wolf writes ("Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for	qemu_chr_open_fd."):
>> +    if (strstart(filename, "nonblock:", &filename))
>> +        block = 0;
> 
> This is definitely wrong, because the need for (non)blocking behaviour
> is determined statically by the code which is using the driver.

It is better than what we have today because I can set non-blocking mode
if I don't want to open the pty immediately and I can set blocking mode
whenever I don't want to lose data and I'm sure to attach to the pty.
This is a decision which your code cannot make statically but one which
is influenced by which behaviour the user wants to have.

Of course, you can always say "we can do even better". But as long as
you don't provide code for non-blocking mode where all data is
guaranteed to be delivered (and still works with a limited buffer...),
it is reasonable to let the user decide.

> Perhaps there should be two writing interfaces, qemu_chr_write and
> qemu_chr_write_(non)block.

And which one will be used if you don't want the user to decide?

Kevin

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

Kevin Wolf wrote:
> Anthony Liguori schrieb:
>> This changes semantics a bit.  Previously, using a pty would guarantee
>> that data is always written as qemu_chr_write does not perform any sort
>> of buffering.
>>
>> Now, that data will be silently dropped instead of causing QEMU to
>> block.  I don't think it's perfectly clear that one behaviour is clearly
>> better than the other.
> 
> Then we need both.
> 
> Add an option to specify non-blocking mode for stdio, pty and pipe in
> qemu_chr_open.  Blocking mode is used by default (maintaining current
> semantics), non-blocking mode if the name is prefixed with nonblock:

Doing that as config option is pusing the problem to the user and a
quite lame way to address the problem.

The IMHO best solution is to send data only if someone is connected to
the slave device (assuming we find a way to figure that).

Independant of that I think the char devices should move to non-blocking
operation.  That depends on the callers being able to handle
qemu_chr_write() doing partial writes and -EAGAIN failures though.
xenconsole code is doing fine here for example, but others probably
depend on the current behavior.  So it should probably the device
emulation code which decides whenever it can run in non-blocking mode or
not.

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Paul Brook]

> >> This changes semantics a bit.  Previously, using a pty would guarantee
> >> that data is always written as qemu_chr_write does not perform any sort
> >> of buffering.
> >>
> >> Now, that data will be silently dropped instead of causing QEMU to
> >> block.  I don't think it's perfectly clear that one behaviour is clearly
> >> better than the other.
> >
> > Then we need both.
> >
> > Add an option to specify non-blocking mode for stdio, pty and pipe in
> > qemu_chr_open.  Blocking mode is used by default (maintaining current
> > semantics), non-blocking mode if the name is prefixed with nonblock:
>
> Doing that as config option is pusing the problem to the user and a
> quite lame way to address the problem.

I agree. Char devices should never drop data when a user is connected. If we 
can't tell whether a user is connected we should never drop data at all, and 
it's the user's responsibility to ensure data doesn't back up.

Paul