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

[Jason Wang <jasowang@redhat.com>]

On 07/19/2013 01:45 PM, Amit Shah wrote:
> 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.

Then the check is not needed here.

>
> 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?

Yes, sounds good.
>
> 		Amit