Revert "usbredir: Disconnect USB device asynchronously"

Submitted by Qiu Wenbo on May 7, 2018, 9:02 a.m.

Details

Message ID 20180507090247.13338-1-qiuwenbo@kylinos.com.cn
State New
Headers show
Series "Revert "usbredir: Disconnect USB device asynchronously"" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Qiu Wenbo May 7, 2018, 9:02 a.m.
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.

Signed-off-by: Qiu Wenbo <qiuwenbo@kylinos.com.cn>
---
 src/channel-usbredir.c | 48 ++++++------------------------------------
 1 file changed, 7 insertions(+), 41 deletions(-)

Patch hide | download patch | download mbox

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
 

Comments

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@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
>
If I remember correctly, this asynchronous disconnection was needed
because in some case, during normal usage, the disconnection was
blocking for seconds (on Windows with usbdk), and if it's not async, it
would block the main loop, and thus the whole GUI (all of this should
have been in the commit log :(
This seems a bit trickier to achieve, but I'd try to block the exit
process until the disconnection thread has finished its job.

Christophe


On Mon, May 07, 2018 at 05:02:47PM +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.
> 
> Signed-off-by: Qiu Wenbo <qiuwenbo@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
>  
> -- 
> 2.17.0
> 
> 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Sat, 2018-05-12 at 11:19 +0800, Qiu Wenbo wrote:
> 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;


I was not able to reproduce with the patch you provide above, but I was
able to reproduce it by inserting a sleep in
_disconnect_device_thread(). As Christophe says, it's rather tricky
problem. I'm looking at it now. Thanks for reporting the issue.

Jonathon


>  
> 
> 
> On Fri, May 11, 2018 at 12:27 AM, Jonathon Jongsma <jjongsma@redhat.c
> om> 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@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(chan
> > > nel,
> > >  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
> > >