From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40853) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTui9-0000jx-PP for qemu-devel@nongnu.org; Tue, 16 Sep 2014 11:30:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTui4-0008N1-AW for qemu-devel@nongnu.org; Tue, 16 Sep 2014 11:30:33 -0400 Received: from smtp.ispras.ru ([83.149.199.79]:60794) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTui4-0008LB-3i for qemu-devel@nongnu.org; Tue, 16 Sep 2014 11:30:28 -0400 Date: Tue, 16 Sep 2014 19:30:21 +0400 (MSK) From: Kirill Batuzov In-Reply-To: <874mw8yu8j.fsf@blackfin.pond.sub.org> Message-ID: References: <1410092666-17405-1-git-send-email-zifeitong@gmail.com> <874mw8yu8j.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Subject: Re: [Qemu-devel] [PATCH] qemu-char: Do not disconnect when there's data for reading List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Gerd Hoffmann , Nikolay Nikolaev , qemu-devel@nongnu.org, Anthony Liguori , Zifei Tong On Tue, 16 Sep 2014, Markus Armbruster wrote: > > Kirill, you added the code being changed. Could you review the patch? > I'll try but this is more about GIOConditions which I do not understand well. See below. Zifei Tong writes: > After commit 812c1057f6175ac9a9829fa2920a2b5783814193 (Handle G_IO_HUP > in tcp_chr_read for tcp chardev), the connection is disconnected when in > G_IO_HUP condition. > > However, it's possible that the channel is in G_IO_IN condition at the > same time, meaning there is data for reading. In that case, the > remaining data is not handled. > > I saw a related bug when running socat in write-only mode, with > > $ echo "quit" | socat -u - UNIX-CONNECT:qemu-monitor > > the monitor won't not run the 'quit' command. > CC: Kirill Batuzov > CC: Nikolay Nikolaev > CC: Anthony Liguori > Signed-off-by: Zifei Tong > --- > qemu-char.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/qemu-char.c b/qemu-char.c > index 1a8d9aa..5018c3a 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -2706,7 +2706,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque) > uint8_t buf[READ_BUF_LEN]; > int len, size; > > - if (cond & G_IO_HUP) { > + if (!(cond & G_IO_IN) && (cond & G_IO_HUP)) { > /* connection closed */ > tcp_chr_disconnect(chr); > return TRUE; I've tried running the above code and watching in debugger what is happening. I wanted to know that tcp_chr_disconnect is called properly so I replaced the 'quit' command with 'help'. From the way code works I have a feeling that we are using some undocumented linux-specific behaviour here. What I saw: - Sometimes all three (G_IO_IN, G_IO_HUP and G_IO_ERR) conditions are asserted, sometimes only two of them (G_IO_IN and G_IO_HUP). - G_IO_IN is *never* de-asserted. Even after all data is depleted it is still up. - When G_IO_ERR is asserted and all data have been read one call to tcp_chr_recv returns -1. Subsequent calls return 0. GIOCondition behaviour in corner cases is puzzling and can differ from OS to OS (commit 812c105 is an example, there also were freebsd-specific bugfixes if I remember correctly). I suggest we remove condition checks completely and use more reliable and better documented source of information - return value of tcp_chr_recv (which is just return value of recvmsg). All we need to do is to handle 'size < 0' and not forget about EAGAIN case. Currently we have a mix of GLib conditions and POSIX return values to handle all cases and we can not do it with GLib alone (I do not know a way to tell if there is data in channel or not when G_IO_HUP is asserted). All these problems were before this patch, but I think it is better to fix it once than add patch over patch fighting GIOCondition's weird behaviour. -- Kirill