[Spice-devel] [PATCH] Revert "usbredir: Disconnect USB device asynchronously"

[Qiu Wenbo]

Yes. The situation can be easily reproduced by the patch below. We have 
noticed it usually happen on some thin clients (without the patch). 
Also, I can reproduce it on my desktop (latest version of Arch linux, 
ryzen 1600x) with debug build of qemu, spice-gtk, and usbredir with 
remote-viewer --spice-debug and gdb attached(also without the patch). 
Step to produce the situation:

1. launch a virtual machine, and redirect a USB mass storage device (or 
something else) to it
2. run udevadmin monitor
3. poweroff your virtual machine by running poweroff in your virtual 
machine

The udevadmin won't tell you there is an ADD uevent for your USB device 
if the situation is produced. Host operating system won't do a driver 
probe automatically without an ADD uevent, and you should do a replug 
to work with the device.

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 0cc5630..2c27825 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -128,6 +128,7 @@ static void 
_channel_reset_finish(SpiceUsbredirChannel *channel)

     spice_usbredir_channel_lock(channel);

+ sleep(1);
     usbredirhost_close(priv->host);
     priv->host = NULL;



On Fri, May 11, 2018 at 12:27 AM, Jonathon Jongsma 
<jjongsma at redhat.com> wrote:
> On Mon, 2018-05-07 at 17:02 +0800, Qiu Wenbo wrote:
>>  This reverts commit 9fbf679453d8dbfe797a738cb536136599d7adab.
>> 
>>  In some cases, remote-viewer will exit before the async function run
>>  in
>>  another thread finish and USB devices redirected to the VM will not
>>  "pop up" to
>>  operation system. For example, a USB disk should be auto mounted
>>  when remote-viewer exit.
> 
> Do you have a reliable way to reproduce this situation?
> 
> 
>> 
>>  Signed-off-by: Qiu Wenbo <qiuwenbo at kylinos.com.cn>
>>  ---
>>   src/channel-usbredir.c | 48 ++++++--------------------------------
>>  ----
>>   1 file changed, 7 insertions(+), 41 deletions(-)
>> 
>>  diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
>>  index 0cc5630..34c4679 100644
>>  --- a/src/channel-usbredir.c
>>  +++ b/src/channel-usbredir.c
>>  @@ -121,54 +121,20 @@ static void
>>  spice_usbredir_channel_init(SpiceUsbredirChannel *channel)
>>   }
>> 
>>   #ifdef USE_USBREDIR
>>  -
>>  -static void _channel_reset_finish(SpiceUsbredirChannel *channel)
>>  -{
>>  -    SpiceUsbredirChannelPrivate *priv = channel->priv;
>>  -
>>  -    spice_usbredir_channel_lock(channel);
>>  -
>>  -    usbredirhost_close(priv->host);
>>  -    priv->host = NULL;
>>  -
>>  -    /* Call set_context to re-create the host */
>>  -    spice_usbredir_channel_set_context(channel, priv->context);
>>  -
>>  -    spice_usbredir_channel_unlock(channel);
>>  -}
>>  -
>>  -static void _channel_reset_cb(GObject *gobject,
>>  -                              GAsyncResult *result,
>>  -                              gpointer user_data)
>>  -{
>>  -    SpiceChannel *spice_channel =  SPICE_CHANNEL(gobject);
>>  -    SpiceUsbredirChannel *channel =
>>  SPICE_USBREDIR_CHANNEL(spice_channel);
>>  -    gboolean migrating = GPOINTER_TO_UINT(user_data);
>>  -    GError *err = NULL;
>>  -
>>  -    _channel_reset_finish(channel);
>>  -
>>  -    SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)-
>>  >channel_reset(spice_channel, migrating);
>>  -
>>  -    spice_usbredir_channel_disconnect_device_finish(channel, 
>> result,
>>  &err);
>>  -    g_object_unref(result);
>>  -}
>>  -
>>   static void spice_usbredir_channel_reset(SpiceChannel *c, gboolean
>>  migrating)
>>   {
>>       SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
>>       SpiceUsbredirChannelPrivate *priv = channel->priv;
>> 
>>       if (priv->host) {
>>  -        if (priv->state == STATE_CONNECTED) {
>>  -            spice_usbredir_channel_disconnect_device_async(channel,
>>  NULL,
>>  -                _channel_reset_cb, GUINT_TO_POINTER(migrating));
>>  -        } else {
>>  -            _channel_reset_finish(channel);
>>  -        }
>>  -    } else {
>>  -        SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)-
>>  >channel_reset(c, migrating);
>>  +        if (priv->state == STATE_CONNECTED)
>>  +            spice_usbredir_channel_disconnect_device(channel);
>>  +        usbredirhost_close(priv->host);
>>  +        priv->host = NULL;
>>  +        /* Call set_context to re-create the host */
>>  +        spice_usbredir_channel_set_context(channel, priv->context);
>>       }
>>  +    SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)-
>>  >channel_reset(c, migrating);
>>   }
>>   #endif
>> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180512/a5306544/attachment-0001.html>