| Message ID | 1587126653-5839-1-git-send-email-sai.pavan.boddu@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | chardev/char-socket: Properly make qio connections non blocking | expand |
Hi On Fri, Apr 17, 2020 at 2:38 PM Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> wrote: > > In tcp_chr_sync_read function, there is a possibility of socket > disconnection during read, then tcp_chr_hup function would clean up > the qio channel pointers(i.e ioc, sioc). > > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> > --- > chardev/char-socket.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index 185fe38..30f2b2b 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -549,11 +549,13 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len) > > qio_channel_set_blocking(s->ioc, true, NULL); > size = tcp_chr_recv(chr, (void *) buf, len); > - qio_channel_set_blocking(s->ioc, false, NULL); But here it calls tcp_chr_recv(). And I can't find cleanup there. Nevertheless, I think this patch should be harmless. I'd ask Daniel to have a second look. > if (size == 0) { > /* connection closed */ > tcp_chr_disconnect(chr); > + return 0; > } > + /* Connection is good */ > + qio_channel_set_blocking(s->ioc, false, NULL); > > return size; > } > -- > 2.7.4 > >
On Fri, Apr 17, 2020 at 03:01:09PM +0200, Marc-André Lureau wrote: > Hi > > On Fri, Apr 17, 2020 at 2:38 PM Sai Pavan Boddu > <sai.pavan.boddu@xilinx.com> wrote: > > > > In tcp_chr_sync_read function, there is a possibility of socket > > disconnection during read, then tcp_chr_hup function would clean up > > the qio channel pointers(i.e ioc, sioc). > > > > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> > > --- > > chardev/char-socket.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > > index 185fe38..30f2b2b 100644 > > --- a/chardev/char-socket.c > > +++ b/chardev/char-socket.c > > @@ -549,11 +549,13 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len) > > > > qio_channel_set_blocking(s->ioc, true, NULL); > > size = tcp_chr_recv(chr, (void *) buf, len); > > - qio_channel_set_blocking(s->ioc, false, NULL); > > But here it calls tcp_chr_recv(). And I can't find cleanup there. > Nevertheless, I think this patch should be harmless. > > I'd ask Daniel to have a second look. I don't see any bug that needs fixing here, and I prefer the current code as it gives confidence that nothing tcp_chr_disconnect does will accidentally block. > > if (size == 0) { > > /* connection closed */ > > tcp_chr_disconnect(chr); > > + return 0; > > } > > + /* Connection is good */ > > + qio_channel_set_blocking(s->ioc, false, NULL); > > > > return size; > > } > > -- > > 2.7.4 > > > > > > > -- > Marc-André Lureau > Regards, Daniel
Hi Daniel, > -----Original Message----- > From: Daniel P. Berrangé <berrange@redhat.com> > Sent: Friday, April 17, 2020 6:44 PM > To: Marc-André Lureau <marcandre.lureau@gmail.com> > Cc: Sai Pavan Boddu <saipava@xilinx.com>; Paolo Bonzini > <pbonzini@redhat.com>; Edgar Iglesias <edgari@xilinx.com>; QEMU <qemu- > devel@nongnu.org> > Subject: Re: [PATCH] chardev/char-socket: Properly make qio connections > non blocking > > On Fri, Apr 17, 2020 at 03:01:09PM +0200, Marc-André Lureau wrote: > > Hi > > > > On Fri, Apr 17, 2020 at 2:38 PM Sai Pavan Boddu > > <sai.pavan.boddu@xilinx.com> wrote: > > > > > > In tcp_chr_sync_read function, there is a possibility of socket > > > disconnection during read, then tcp_chr_hup function would clean up > > > the qio channel pointers(i.e ioc, sioc). > > > > > > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> > > > --- > > > chardev/char-socket.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c index > > > 185fe38..30f2b2b 100644 > > > --- a/chardev/char-socket.c > > > +++ b/chardev/char-socket.c > > > @@ -549,11 +549,13 @@ static int tcp_chr_sync_read(Chardev *chr, > > > const uint8_t *buf, int len) > > > > > > qio_channel_set_blocking(s->ioc, true, NULL); > > > size = tcp_chr_recv(chr, (void *) buf, len); > > > - qio_channel_set_blocking(s->ioc, false, NULL); > > > > But here it calls tcp_chr_recv(). And I can't find cleanup there. > > Nevertheless, I think this patch should be harmless. > > > > I'd ask Daniel to have a second look. > > I don't see any bug that needs fixing here, and I prefer the current code as it > gives confidence that nothing tcp_chr_disconnect does will accidentally > block. [Sai Pavan Boddu] The issue is triggered when 'tcp_chr_hup' callback, is dispatched when socket hung up during a blocking read. Which also calls tcp_chr_disconnect and cleans up ioc, sioc pointers. Later below line segfaults due to null pointers " qio_channel_set_blocking(s->ioc, false, NULL); " Regards, Sai Pavan > > > > > if (size == 0) { > > > /* connection closed */ > > > tcp_chr_disconnect(chr); > > > + return 0; > > > } > > > + /* Connection is good */ > > > + qio_channel_set_blocking(s->ioc, false, NULL); > > > > > > return size; > > > } > > > -- > > > 2.7.4 > > > > > > > > > > > > -- > > Marc-André Lureau > > > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange > :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange > :|
diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 185fe38..30f2b2b 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -549,11 +549,13 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len) qio_channel_set_blocking(s->ioc, true, NULL); size = tcp_chr_recv(chr, (void *) buf, len); - qio_channel_set_blocking(s->ioc, false, NULL); if (size == 0) { /* connection closed */ tcp_chr_disconnect(chr); + return 0; } + /* Connection is good */ + qio_channel_set_blocking(s->ioc, false, NULL); return size; }
In tcp_chr_sync_read function, there is a possibility of socket disconnection during read, then tcp_chr_hup function would clean up the qio channel pointers(i.e ioc, sioc). Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> --- chardev/char-socket.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)