> > Hi, > > On Wed, Sep 26, 2018 at 06:17:34AM -0400, Frediano Ziglio wrote: > > > > > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > > > This should not change code behavior, only add some comments. > > > > > > This is a preparatory patch for bug below, introduced with: > > > > > > commit 9fbf679453d8dbfe797a738cb536136599d7adab > > > Author: Kirill Moizik <kmoizik@xxxxxxxxxx> > > > Date: Tue Mar 8 16:05:57 2016 +0200 > > > > > > usbredir: Disconnect USB device asynchronously > > > > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1625550 > > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx> > > > --- > > > src/channel-usbredir.c | 27 +++++++++++++++++++-------- > > > 1 file changed, 19 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c > > > index 1d9c380..7f6e2e3 100644 > > > --- a/src/channel-usbredir.c > > > +++ b/src/channel-usbredir.c > > > @@ -158,16 +158,27 @@ 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 { > > > + /* Host isn't running, just reset */ > > > + if (!priv->host) { > > > SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)->channel_reset(c, > > > migrating); > > > + return; > > > } > > > + > > > + /* Host is running, so we might need to disconnect the usb devices > > > async. > > > + * This should not block channel_reset() otherwise we might run in > > > reconnection > > > + * problems such as > > > https://bugzilla.redhat.com/show_bug.cgi?id=1625550 > > > + * No operation from here on should rely on SpiceChannel as its > > > coroutine > > > + * might be terminated. */ > > > + > > > + if (priv->state == STATE_CONNECTED) { > > > + /* FIXME: We should add GCancellable here */ > > > > I still think that not chaining channel_reset here is a bug. > > But this is not a regression. > > While I agree, I have to understand a bit more of the usb side to > properly fix it. Commit 9fbf679453d8db does not provide much > context, I'll have to dig mailing list for the rationale plus > find a way to reset device async while resetting SpiceChannel > here. > I did a similar research. I don't remember to have found much on the mailing list but the commit is in a series and some other commit has much more comments on them with some explanation. I tried also to detach the usb state from the channel but was a bit too tight with the channel. > Well, all might be fixed with the recent rework there with cd > redir... More things to understand. > I don't know > > Why you want to support GCancellable? Looks like more a TODO > > than a FIXME. > > I'll change the comment to > > /* FIXME: We should chain-up parent's channel-reset here */ > > Sounds good? > Yes > > > > > + spice_usbredir_channel_disconnect_device_async(channel, NULL, > > > + _channel_reset_cb, GUINT_TO_POINTER(migrating)); > > > + return; > > > + } > > > + > > > + /* FIXME: This does not chain-up with parent's channel-reset, which > > > is a > > > must */ > > > + _channel_reset_finish(channel); > > > } > > > #endif > > > > > > > Otherwise, > > > > Acked-by: Frediano Ziglio <fziglio@xxxxxxxxxx> > > > > Frediano > > Thanks, > > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel