[Qemu-devel] [PATCH 1/2] unix_write: don't block on non-blocking file handles.

[Qemu-devel] [PATCH 1/2] unix_write: don't block on non-blocking file handles.

[Gerd Hoffmann]

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

diff --git a/vl.c b/vl.c
index 0e98eef..587b91c 100644
--- a/vl.c
+++ b/vl.c
@@ -2119,14 +2119,24 @@ void socket_set_nonblock(int fd)
 
 static int unix_write(int fd, const uint8_t *buf, int len1)
 {
+    int nonblock = fcntl(fd, F_GETFL) & O_NONBLOCK;
     int ret, len;
 
     len = len1;
     while (len > 0) {
         ret = write(fd, buf, len);
         if (ret < 0) {
-            if (errno != EINTR && errno != EAGAIN)
+            if (errno == EINTR) {
+		continue;
+	    } else if (errno == EAGAIN) {
+		if (!nonblock)
+		    continue;
+		if (len1 != len)
+		    break; /* partial write, return written bytes */
+		return -1;
+	    } else {
                 return -1;
+	    }
         } else if (ret == 0) {
             break;
         } else {
-- 
1.5.4.1

[Qemu-devel] [PATCH 2/2] open ptys in non-blocking mode.

[Gerd Hoffmann]

Otherwise qemu will hang in case nobody connects to the pty and the
guests prints enougth messages to fill up the buffer (which is 4k
in linux).

Downside is that data may get lost in case the reader is too slow.

Ideally we could detect whenever someone is connected to the other end
of the pseudo tty and write data only in connected mode (like it is done
for tcp/telnet).  I'm not aware of any way to accomplish that though.

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

diff --git a/vl.c b/vl.c
index 587b91c..adc8f5f 100644
--- a/vl.c
+++ b/vl.c
@@ -2468,6 +2468,7 @@ static CharDriverState *qemu_chr_open_pty(void)
     /* Set raw attributes on the pty. */
     cfmakeraw(&tty);
     tcsetattr(slave_fd, TCSAFLUSH, &tty);
+    socket_set_nonblock(master_fd);
 
     fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
     return qemu_chr_open_fd(master_fd, master_fd);
-- 
1.5.4.1

Re: [Qemu-devel] [PATCH 2/2] open ptys in non-blocking mode.

[Kevin Wolf]

[Crossposting to xen-devel]

Ian, we need something like this for qemu-xen (or ioemu-remote or
whatever it's called now). Currently you must attach to the console of a
domain, otherwise it won't boot up and keep hanging in a blocking write
because the buffer is full.

The old ioemu had a hack in unix_write (doing a select before the write)
which you didn't merge into qemu-xen. In fact, I noticed that you even
removed that function entirely and I'm wondering why.

Kevin

Gerd Hoffmann schrieb:
> Otherwise qemu will hang in case nobody connects to the pty and the
> guests prints enougth messages to fill up the buffer (which is 4k
> in linux).
> 
> Downside is that data may get lost in case the reader is too slow.
> 
> Ideally we could detect whenever someone is connected to the other end
> of the pseudo tty and write data only in connected mode (like it is done
> for tcp/telnet).  I'm not aware of any way to accomplish that though.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  vl.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 587b91c..adc8f5f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2468,6 +2468,7 @@ static CharDriverState *qemu_chr_open_pty(void)
>      /* Set raw attributes on the pty. */
>      cfmakeraw(&tty);
>      tcsetattr(slave_fd, TCSAFLUSH, &tty);
> +    socket_set_nonblock(master_fd);
>  
>      fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
>      return qemu_chr_open_fd(master_fd, master_fd);

Re: [Qemu-devel] [PATCH 2/2] open ptys in non-blocking mode.

[Gerd Hoffmann]

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

Kevin Wolf wrote:
> [Crossposting to xen-devel]
> 
> Ian, we need something like this for qemu-xen (or ioemu-remote or
> whatever it's called now). Currently you must attach to the console of a
> domain, otherwise it won't boot up and keep hanging in a blocking write
> because the buffer is full.
> 
> The old ioemu had a hack in unix_write (doing a select before the write)
> which you didn't merge into qemu-xen. In fact, I noticed that you even
> removed that function entirely and I'm wondering why.

For completeness:  You also need the attached patch for unix_write,
otherwise you'll end up with qemu burning cpu cycles.  If you can't
write to a non-blocking file handle the write will instantly return with
-EAGAIN.  Calling it again of course doesn't change the result, so
better don't do that ...

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

[-- Attachment #2: 0002-unix_write-don-t-block-on-non-blocking-file-handles.patch --]
[-- Type: text/x-patch, Size: 1128 bytes --]

>From d9454802fa2105bc399518eebfa1e1415ff07143 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 9 Jul 2008 09:41:31 +0200
Subject: [PATCH 02/12] unix_write: don't block on non-blocking file handles.


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

diff --git a/vl.c b/vl.c
index c8db579..2e2d883 100644
--- a/vl.c
+++ b/vl.c
@@ -2116,14 +2116,24 @@ void socket_set_nonblock(int fd)
 
 static int unix_write(int fd, const uint8_t *buf, int len1)
 {
+    int nonblock = fcntl(fd, F_GETFL) & O_NONBLOCK;
     int ret, len;
 
     len = len1;
     while (len > 0) {
         ret = write(fd, buf, len);
         if (ret < 0) {
-            if (errno != EINTR && errno != EAGAIN)
+            if (errno == EINTR) {
+		continue;
+	    } else if (errno == EAGAIN) {
+		if (!nonblock)
+		    continue;
+		if (len1 != len)
+		    break; /* partial write, return written bytes */
+		return -1;
+	    } else {
                 return -1;
+	    }
         } else if (ret == 0) {
             break;
         } else {
-- 
1.5.4.1

Re: [Qemu-devel] [PATCH 2/2] open ptys in non-blocking mode.

[Kevin Wolf]

Gerd Hoffmann schrieb:
> Kevin Wolf wrote:
>> [Crossposting to xen-devel]
>>
>> Ian, we need something like this for qemu-xen (or ioemu-remote or
>> whatever it's called now). Currently you must attach to the console of a
>> domain, otherwise it won't boot up and keep hanging in a blocking write
>> because the buffer is full.
>>
>> The old ioemu had a hack in unix_write (doing a select before the write)
>> which you didn't merge into qemu-xen. In fact, I noticed that you even
>> removed that function entirely and I'm wondering why.
> 
> For completeness:  You also need the attached patch for unix_write,
> otherwise you'll end up with qemu burning cpu cycles.  If you can't
> write to a non-blocking file handle the write will instantly return with
> -EAGAIN.  Calling it again of course doesn't change the result, so
> better don't do that ...

As I metioned above, in qemu-xen there is no unix_write. It calls
write() directly instead. But if we wanted to re-introduce unix_write to
be closer to upstream qemu (I still don't know why unix_write was
dropped in the first place for qemu-xen - Ian?) you're right, of course.

Kevin

Re: [Qemu-devel] [PATCH 2/2] open ptys in non-blocking mode.

[Ian Jackson]

Kevin Wolf writes ("Re: [Qemu-devel] [PATCH 2/2] open ptys in non-blocking mode."):
> Ian, we need something like this for qemu-xen (or ioemu-remote or
> whatever it's called now). Currently you must attach to the console of a
> domain, otherwise it won't boot up and keep hanging in a blocking write
> because the buffer is full.

Yes, that's right.  See my other messages, including the patches which
will be arriving shortly ...

> The old ioemu had a hack in unix_write (doing a select before the write)
> which you didn't merge into qemu-xen. In fact, I noticed that you even
> removed that function entirely and I'm wondering why.

We have qemu_{read,write} instead, as I wrote in January.  Those make
it easy to have good error handling.  Sadly my patch from January
wasn't accepted upstream.

Ian.

Re: [Qemu-devel] [PATCH 2/2] open ptys in non-blocking mode.

[Ian Jackson]

Gerd Hoffmann writes ("[Qemu-devel] [PATCH 2/2] open ptys in non-blocking mode."):
> Otherwise qemu will hang in case nobody connects to the pty and the
> guests prints enougth messages to fill up the buffer (which is 4k
> in linux).

This patch is correct, but I don't think it goes far enough.  There
are a couple of other cases which need fixing too.

I will send a patch series about this.

Ian.

Re: [Qemu-devel] [PATCH 1/2] unix_write: don't block on non-blocking file handles.

[Ian Jackson]

Gerd Hoffmann writes ("[Qemu-devel] [PATCH 1/2] unix_write: don't block on non-blocking file handles."):
> -            if (errno != EINTR && errno != EAGAIN)
> +            if (errno == EINTR) {
> +		continue;
> +	    } else if (errno == EAGAIN) {
> +		if (!nonblock)
> +		    continue;

I think this patch is not harmful.  However, it is I think always
correct to return EAGAIN to the caller.  This is what I have in the
Xen qemu tree.

I submitted a patch to this effect, which replaced unix_{read,write}
with qemu_{read,write}, amongst other error-handling changes, on the
25th of January, under the subject line `[PATCH] check return value
from read() and write() properly'

There was a small amount of discussion including some critical
comments from Anthony Ligouri but I think I addressed all of those at
the time.  Sadly the patch did not get accepted after that.

Ian.