[Spice-devel] PATCH: virtio_console: Fix poll blocking even though there is data to read

[Amit Shah]

On (Thu) Sep 16 2010 [11:52:01], Amit Shah wrote:
> 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.

Uh, sorry for blabbering.

will_read_block() was written to be used in poll(), and the comment says
as much.  I should've used it right from the start.  Hans's first patch
was correct, I'll pick that up and that addresses all the "issues" that
are present.

Sorry again!

		Amit