[Spice-devel] [PATCH v6 10/14] usbredir: Disconnect USB device asynchronously

[Dmitry Fleytman]

> On 1 Mar 2016, at 21:25 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> 
> On Sun, 2016-02-28 at 11:54 +0200, Dmitry Fleytman wrote:
>> From: Kirill Moizik <kmoizik at redhat.com>
>> 
>> Signed-off-by: Kirill Moizik <kmoizik at redhat.com>
>> Signed-off-by: Dmitry Fleytman <dfleytma at redhat.com>
>> ---
>> src/channel-usbredir.c | 38 +++++++++++++++++++++++++++++++-------
>> 1 file changed, 31 insertions(+), 7 deletions(-)
>> 
>> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
>> index ebf9fce..3a10823 100644
>> --- a/src/channel-usbredir.c
>> +++ b/src/channel-usbredir.c
>> @@ -114,20 +114,44 @@ static void
>> spice_usbredir_channel_init(SpiceUsbredirChannel *channel)
>> }
>> 
>> #ifdef USE_USBREDIR
>> +
>> +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);
>> +    SpiceUsbredirChannelPrivate *priv = channel->priv;
>> +    gboolean migrating = GPOINTER_TO_UINT(user_data);
>> +    GError *err = NULL;
>> +
>> +    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_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
>> ->channel_reset(spice_channel, migrating);
>> +
>> +    spice_usbredir_channel_unlock(channel);
>> +
>> +    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(channel);
>> -        usbredirhost_close(priv->host);
>> -        priv->host = NULL;
>> -        /* Call set_context to re-create the host */
>> -        spice_usbredir_channel_set_context(channel, priv->context);
>> +        if (priv->state == STATE_CONNECTED) {
>> +            spice_usbredir_channel_disconnect_device_async(channel, NULL,
>> +                _channel_reset_cb, GUINT_TO_POINTER(migrating));
> 
> I don't think I noticed this on the previous review, but it seems there is a
> change in behavior introduced here. Previously, if priv->state was not
> STATE_CONNECTED, it would omit the call to
> spice_usbredir_channel_disconnect_device(), but would proceed to call
> usbredirhost_close(), etc. In the new version, these additional calls are done
> in the async callback. And the async function is only executed if state is
> STATE_CONNECTED. I'm afraid I don't know enough about this code to know whether
> that's going to be an issue. Was this change intentional?

You’re right. This change is unintentional, the original behaviour is preserved in the next version.

> 
>> +        }
>> +    } else {
>> +        SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
>> ->channel_reset(c, migrating);
>>     }
>> -    SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
>> ->channel_reset(c, migrating);
>> }
>> #endif
>> 
> 
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com <mailto:jjongsma at redhat.com>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160308/0430d033/attachment.html>