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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/virtualization
