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 <zifeit...@gmail.com> 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 <batuz...@ispras.ru>
> CC: Nikolay Nikolaev <n.nikol...@virtualopensystems.com>
> CC: Anthony Liguori <aligu...@amazon.com>
> Signed-off-by: Zifei Tong <zifeit...@gmail.com>
> ---
> 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
