[Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Don Slutz]

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 qemu-char.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-char.c b/qemu-char.c
index ef84b53..6eec1d2 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1387,6 +1387,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
     }
 
     close(slave_fd);
+    qemu_set_nonblock(master_fd);
 
     chr = qemu_chr_alloc();
 
-- 
1.8.4

[Qemu-devel] [PATCH 1/1] Do not hang on full PTY

[Don Slutz]

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 qemu-char.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-char.c b/qemu-char.c
index ef84b53..6eec1d2 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1387,6 +1387,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
     }
 
     close(slave_fd);
+    qemu_set_nonblock(master_fd);
 
     chr = qemu_chr_alloc();
 
-- 
1.8.4

Re: [Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Michael Tokarev]

22.12.2014 18:04, Don Slutz wrote:

> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1387,6 +1387,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>      }
>  
>      close(slave_fd);
> +    qemu_set_nonblock(master_fd);
>  
>      chr = qemu_chr_alloc();


Hm.  I'm not sure at all this is a trivial change.  While the
patch itself is trivial indeed, it changes behavour of the file
descriptor significantly.  Are all the places where this fd is
subsequently used prepared for it being non-blocking?  Oh well... ;)

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Michael Tokarev]

22.12.2014 18:04, Don Slutz wrote:

> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1387,6 +1387,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>      }
>  
>      close(slave_fd);
> +    qemu_set_nonblock(master_fd);
>  
>      chr = qemu_chr_alloc();


Hm.  I'm not sure at all this is a trivial change.  While the
patch itself is trivial indeed, it changes behavour of the file
descriptor significantly.  Are all the places where this fd is
subsequently used prepared for it being non-blocking?  Oh well... ;)

Thanks,

/mjt

Re: [Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Don Slutz]

On 12/29/14 04:59, Michael Tokarev wrote:
> 22.12.2014 18:04, Don Slutz wrote:
> 
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -1387,6 +1387,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>>      }
>>  
>>      close(slave_fd);
>> +    qemu_set_nonblock(master_fd);
>>  
>>      chr = qemu_chr_alloc();
> 
> 
> Hm.  I'm not sure at all this is a trivial change.  While the
> patch itself is trivial indeed, it changes behavour of the file
> descriptor significantly.  Are all the places where this fd is
> subsequently used prepared for it being non-blocking?  Oh well... ;)

I was not sure on this being trivial also, but it looked like it could
be to me.  The uses of this FD all looked that they handle non-blocking.

Here are the calls to qemu_set_nonblock in qemu-char.c (including this
add) :

b qemu-char.c             qemu_chr_open_fd           1070
qemu_set_nonblock(fd_out);
c qemu-char.c             qemu_chr_open_stdio        1166
qemu_set_nonblock(0);
d qemu-char.c             qemu_chr_open_pty          1390
qemu_set_nonblock(master_fd);
e qemu-char.c             tcp_chr_add_client         2952
qemu_set_nonblock(fd);
f qemu-char.c             qmp_chardev_open_serial    4060
qemu_set_nonblock(fd);
g qemu-char.c             qmp_chardev_open_socket    4172
qemu_set_nonblock(s->listen_fd);

So there are many cases where the FD is non-blocking already.

Hope this info helps.

> 
> Thanks,
> 
> /mjt
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Don Slutz]

On 12/29/14 04:59, Michael Tokarev wrote:
> 22.12.2014 18:04, Don Slutz wrote:
> 
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -1387,6 +1387,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>>      }
>>  
>>      close(slave_fd);
>> +    qemu_set_nonblock(master_fd);
>>  
>>      chr = qemu_chr_alloc();
> 
> 
> Hm.  I'm not sure at all this is a trivial change.  While the
> patch itself is trivial indeed, it changes behavour of the file
> descriptor significantly.  Are all the places where this fd is
> subsequently used prepared for it being non-blocking?  Oh well... ;)

I was not sure on this being trivial also, but it looked like it could
be to me.  The uses of this FD all looked that they handle non-blocking.

Here are the calls to qemu_set_nonblock in qemu-char.c (including this
add) :

b qemu-char.c             qemu_chr_open_fd           1070
qemu_set_nonblock(fd_out);
c qemu-char.c             qemu_chr_open_stdio        1166
qemu_set_nonblock(0);
d qemu-char.c             qemu_chr_open_pty          1390
qemu_set_nonblock(master_fd);
e qemu-char.c             tcp_chr_add_client         2952
qemu_set_nonblock(fd);
f qemu-char.c             qmp_chardev_open_serial    4060
qemu_set_nonblock(fd);
g qemu-char.c             qmp_chardev_open_socket    4172
qemu_set_nonblock(s->listen_fd);

So there are many cases where the FD is non-blocking already.

Hope this info helps.

> 
> Thanks,
> 
> /mjt
> 

Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/1] Do not hang on full PTY

[Peter Maydell]

On 29 December 2014 at 20:27, Don Slutz <dslutz@verizon.com> wrote:
> I was not sure on this being trivial also, but it looked like it could
> be to me.  The uses of this FD all looked that they handle non-blocking.

Does g_io_channel_read_chars() definitely return G_IO_STATUS_NORMAL
(and not, say, G_IO_STATUS_AGAIN) for an attempted read on a non-blocking
fd with no data? Otherwise pty_chr_read() is going to call
pty_chr_state(chr, 0) which I think means "the other end has hung up"
and will take the fd out of the main loop's poll set.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Peter Maydell]

On 29 December 2014 at 20:27, Don Slutz <dslutz@verizon.com> wrote:
> I was not sure on this being trivial also, but it looked like it could
> be to me.  The uses of this FD all looked that they handle non-blocking.

Does g_io_channel_read_chars() definitely return G_IO_STATUS_NORMAL
(and not, say, G_IO_STATUS_AGAIN) for an attempted read on a non-blocking
fd with no data? Otherwise pty_chr_read() is going to call
pty_chr_state(chr, 0) which I think means "the other end has hung up"
and will take the fd out of the main loop's poll set.

thanks
-- PMM

Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/1] Do not hang on full PTY

[Don Slutz]

On 12/29/14 18:41, Peter Maydell wrote:
> On 29 December 2014 at 20:27, Don Slutz <dslutz@verizon.com> wrote:
>> I was not sure on this being trivial also, but it looked like it could
>> be to me.  The uses of this FD all looked that they handle non-blocking.
> Does g_io_channel_read_chars() definitely return G_IO_STATUS_NORMAL
> (and not, say, G_IO_STATUS_AGAIN) for an attempted read on a non-blocking
> fd with no data?

The only time I know of to get here in that state, is when the other end 
disconnects.
Normally pty_chr_read will only be called when there is at least 1 
character to read or
a state change.

>   Otherwise pty_chr_read() is going to call
> pty_chr_state(chr, 0) which I think means "the other end has hung up"
> and will take the fd out of the main loop's poll set.

Yes, that is correct.  But it only happens when the other end disconnects.
pty_chr_timer() also is involved here, so on a reconnect, the polling is 
re-enabled.

    -Don Slutz

>
> thanks
> -- PMM

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Don Slutz]

On 12/29/14 18:41, Peter Maydell wrote:
> On 29 December 2014 at 20:27, Don Slutz <dslutz@verizon.com> wrote:
>> I was not sure on this being trivial also, but it looked like it could
>> be to me.  The uses of this FD all looked that they handle non-blocking.
> Does g_io_channel_read_chars() definitely return G_IO_STATUS_NORMAL
> (and not, say, G_IO_STATUS_AGAIN) for an attempted read on a non-blocking
> fd with no data?

The only time I know of to get here in that state, is when the other end 
disconnects.
Normally pty_chr_read will only be called when there is at least 1 
character to read or
a state change.

>   Otherwise pty_chr_read() is going to call
> pty_chr_state(chr, 0) which I think means "the other end has hung up"
> and will take the fd out of the main loop's poll set.

Yes, that is correct.  But it only happens when the other end disconnects.
pty_chr_timer() also is involved here, so on a reconnect, the polling is 
re-enabled.

    -Don Slutz

>
> thanks
> -- PMM

Re: [Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Paolo Bonzini]

On 30/12/2014 00:41, Peter Maydell wrote:
> On 29 December 2014 at 20:27, Don Slutz <dslutz@verizon.com> wrote:
>> I was not sure on this being trivial also, but it looked like it could
>> be to me.  The uses of this FD all looked that they handle non-blocking.
> 
> Does g_io_channel_read_chars() definitely return G_IO_STATUS_NORMAL
> (and not, say, G_IO_STATUS_AGAIN) for an attempted read on a non-blocking
> fd with no data?

It should return G_IO_STATUS_AGAIN.  However, pty_chr_read() won't be
called in the first place because the fd won't be readable and thus the
chr->fd_in_tag GSource won't fire.

I think things more or less work right now just because PTYs are
buffered in the kernel and there's no network involved, but Don's patch
is good.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Michael, let me know if you're applying it yourself.

Paolo

> Otherwise pty_chr_read() is going to call
> pty_chr_state(chr, 0) which I think means "the other end has hung up"
> and will take the fd out of the main loop's poll set.
> 
> thanks
> -- PMM
> 
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Paolo Bonzini]

On 30/12/2014 00:41, Peter Maydell wrote:
> On 29 December 2014 at 20:27, Don Slutz <dslutz@verizon.com> wrote:
>> I was not sure on this being trivial also, but it looked like it could
>> be to me.  The uses of this FD all looked that they handle non-blocking.
> 
> Does g_io_channel_read_chars() definitely return G_IO_STATUS_NORMAL
> (and not, say, G_IO_STATUS_AGAIN) for an attempted read on a non-blocking
> fd with no data?

It should return G_IO_STATUS_AGAIN.  However, pty_chr_read() won't be
called in the first place because the fd won't be readable and thus the
chr->fd_in_tag GSource won't fire.

I think things more or less work right now just because PTYs are
buffered in the kernel and there's no network involved, but Don's patch
is good.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Michael, let me know if you're applying it yourself.

Paolo

> Otherwise pty_chr_read() is going to call
> pty_chr_state(chr, 0) which I think means "the other end has hung up"
> and will take the fd out of the main loop's poll set.
> 
> thanks
> -- PMM
> 
> 

Re: [Qemu-trivial] [PATCH 1/1] Do not hang on full PTY

[Michael Tokarev]

Applied to -trivial, thank you!

/mjt

Re: [Qemu-devel] [PATCH 1/1] Do not hang on full PTY

[Michael Tokarev]

Applied to -trivial, thank you!

/mjt