Re: PATCH: virtio_console: Fix poll blocking even though there is data to read

[Amit Shah <amit.shah@redhat.com>]

On (Thu) Sep 16 2010 [15:32:54], Rusty Russell wrote:
> On Wed, 15 Sep 2010 11:16:24 pm Amit Shah wrote:
> > On (Wed) Sep 15 2010 [15:37:21], Hans de Goede wrote:
> > > >>--- linux-2.6.35.x86_64/drivers/char/virtio_console.c~	2010-08-02 00:11:14.000000000 +0200
> > > >>+++ linux-2.6.35.x86_64/drivers/char/virtio_console.c	2010-09-15 13:39:29.043505000 +0200
> > > >>@@ -642,7 +642,7 @@ static unsigned int port_fops_poll(struc
> > > >>  	poll_wait(filp,&port->waitqueue, wait);
> > > >>
> > > >>  	ret = 0;
> > > >>-	if (port->inbuf)
> > > >>+	if (!will_read_block(port))
> > > >
> > > >Looks correct, but this should be
> > > >
> > > >	if (port_has_data(port))
> > > >
> > > >instead.
> > > 
> > > That certainly works for me (as in will still fix the bug I'm hitting), but
> > > quoting from "man 2 select":
> > > 
> > >        Three  independent  sets of file descriptors are watched.  Those listed
> > >        in readfds will be watched to see if characters  become  available  for
> > >        reading  (more  precisely, to see if a read will not block; in particu‐
> > >        lar, a file descriptor is also ready on end-of-file)
> > > 
> > > Notice the "a file descriptor is also ready on end-of-file", and
> > > port_fops_read treats the host not being connected as eof (it returns 0
> > > in that case). So from an API pov I'm not sure what is correct.
> > 
> > poll(2) says:
> > 
> >               POLLIN There is data to read.
> > 
> > That makes it simple.
> 
> That's a documentation bug.  On a pipe, POLLIN is set when the other end
> closes.  read() then returns 0 immediately.

Currently we don't set POLLIN when host goes down.  I'll do a second
patch for that.

> poll() sets POLLIN when read() won't block, and people count on it.

Yes, that's the behaviour with Hans's new patch as well -- that's not
changing.

The will_read_block() function (and the comment on top of it) are
causing this confusion:


/* The condition that must be true for polling to end */
static bool will_read_block(struct port *port)
{
	if (!port->guest_connected) {
		/* Port got hot-unplugged. Let's exit. */
		return false;
	}
	return !port_has_data(port) && port->host_connected;
}

This function is only called to unblock a blocking read() call.  So the
comment there has to be changed to read 'waiting' to end instead of
'polling' to end.

read() does return 0 immediately when the other end is not connected
(and there's no data to read).

In effect, we need:
- Hans's patch
- a patch to set POLLIN when host goes down (in addition to POLLHUP and
  SIGIO)
- a patch to change the comment for will_read_block.

Thanks,

		Amit