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

[Jonathon Jongsma]

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 at 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 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(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
> > >