Re: [PATCH 04/10] virtio: console: return -ENODEV on all read operations after unplug

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

On (Fri) 19 Jul 2013 [13:07:49], Jason Wang wrote:
> On 07/19/2013 04:16 AM, Amit Shah wrote:
> > If a port gets unplugged while a user is blocked on read(), -ENODEV is
> > returned.  However, subsequent read()s returned 0, indicating there's no
> > host-side connection (but not indicating the device went away).
> >
> > This also happened when a port was unplugged and the user didn't have
> > any blocking operation pending.  If the user didn't monitor the SIGIO
> > signal, they won't have a chance to find out if the port went away.
> >
> > Fix by returning -ENODEV on all read()s after the port gets unplugged.
> > write() already behaves this way.
> >
> > CC: <stable@vger.kernel.org>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/char/virtio_console.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index 6bf0df3..a39702a 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -749,6 +749,10 @@ static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
> >  
> >  	port = filp->private_data;
> >  
> > +	/* Port is hot-unplugged. */
> > +	if (!port->guest_connected)
> > +		return -ENODEV;
> > +
> 
> What if the port is hot-unplugged after this check? Should we serialize
> the hot plug with inbuf_lock here?

If that happens, port_has_data() returns false, and the later
functions return appropriately, as unplug_port() clears out all the
data.

However, I spotted a couple of things that need fixing:

1. In the condition you describe above, port_has_data will return
false, and host_connected will be false as well, and fops_read() will
return 0 instead of -ENODEV once.  Subsequent reads will return
-ENODEV.

2. get_inbuf(), which is called by port_has_data() accesses the vqs
even if the port is unplugged.  The vqs are still available, and won't
have data in them (since the port is unplugged), but it's best to not
rely on such behaviour.  For the next merge window, I'll add extra
state, port_(un)plugged to track this instead of abusing
guest_connected.

That also simplifies the path for later if we get vq hot-plug as well.

I think the current code will behave satisfactorily for now; what do
you think?

		Amit