Message ID | 1410092666-17405-1-git-send-email-zifeitong@gmail.com |
---|---|
State | New |
Headers | show |
Friendly ping. Also CC to qemu-trivial. On Sun, Sep 7, 2014 at 8:24 PM, Zifei Tong <zifeitong@gmail.com> wrote: > 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 <batuzovk@ispras.ru> > CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> > CC: Anthony Liguori <aliguori@amazon.com> > Signed-off-by: Zifei Tong <zifeitong@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; > -- > 2.1.0 >
Cc'ing Gerd for additional chardev expertise. Zifei Tong <zifeitong@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. Reproduced. Is this a regression caused by command 812c105? > CC: Kirill Batuzov <batuzovk@ispras.ru> > CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> > CC: Anthony Liguori <aliguori@amazon.com> > Signed-off-by: Zifei Tong <zifeitong@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; Kirill, you added the code being changed. Could you review the patch?
On Tue, Sep 16, 2014 at 2:06 PM, Markus Armbruster <armbru@redhat.com> wrote: > Cc'ing Gerd for additional chardev expertise. > > Zifei Tong <zifeitong@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. > > Reproduced. > > Is this a regression caused by command 812c105? The first bad commit is cdaa86 ("Add G_IO_HUP handler for socket chardev"), which is reverted and reimplemented by 812c105. >> CC: Kirill Batuzov <batuzovk@ispras.ru> >> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> >> CC: Anthony Liguori <aliguori@amazon.com> >> Signed-off-by: Zifei Tong <zifeitong@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; > > Kirill, you added the code being changed. Could you review the patch?
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 <zifeitong@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 <batuzovk@ispras.ru> > CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> > CC: Anthony Liguori <aliguori@amazon.com> > Signed-off-by: Zifei Tong <zifeitong@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.
On Tue, Sep 16, 2014 at 11:30 PM, Kirill Batuzov <batuzovk@ispras.ru> wrote: > 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 <zifeitong@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 <batuzovk@ispras.ru> >> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> >> CC: Anthony Liguori <aliguori@amazon.com> >> Signed-off-by: Zifei Tong <zifeitong@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; Hi Kirill, Thanks for your review and detailed analysis. > 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. This sounds good to me. I'll send a V2 patch based on this. Thanks, Zifei > 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
16.09.2014 09:31, Zifei Tong wrote: > Friendly ping. > > Also CC to qemu-trivial. There was at least 2 subsequent versions of this down the line, a v2 and a v3. (Just to show why I haven't applied it to -trivial). Thanks, /mjt > On Sun, Sep 7, 2014 at 8:24 PM, Zifei Tong <zifeitong@gmail.com> wrote: >> 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 <batuzovk@ispras.ru> >> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> >> CC: Anthony Liguori <aliguori@amazon.com> >> Signed-off-by: Zifei Tong <zifeitong@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; >> -- >> 2.1.0 >> >
On Wed, Sep 24, 2014 at 11:52:37AM +0400, Michael Tokarev wrote: > 16.09.2014 09:31, Zifei Tong wrote: > > Friendly ping. > > > > Also CC to qemu-trivial. > > There was at least 2 subsequent versions of this down the line, > a v2 and a v3. (Just to show why I haven't applied it to -trivial). > > Thanks, > > /mjt It's also far from trivial. I don't see why it applies there. > > On Sun, Sep 7, 2014 at 8:24 PM, Zifei Tong <zifeitong@gmail.com> wrote: > >> 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 <batuzovk@ispras.ru> > >> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> > >> CC: Anthony Liguori <aliguori@amazon.com> > >> Signed-off-by: Zifei Tong <zifeitong@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; > >> -- > >> 2.1.0 > >> > > >
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;
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 <batuzovk@ispras.ru> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> CC: Anthony Liguori <aliguori@amazon.com> Signed-off-by: Zifei Tong <zifeitong@gmail.com> --- qemu-char.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)