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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
