| Message ID | 1419260640-2922-1-git-send-email-dslutz@verizon.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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
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 > >
Applied to -trivial, thank you! /mjt
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();
Signed-off-by: Don Slutz <dslutz@verizon.com> --- qemu-char.c | 1 + 1 file changed, 1 insertion(+)