[Spice-devel] [spice-gtk v3 3/4] channel-usbredir: Add FIXMEs on channel-reset issues

[Frediano Ziglio]

> 
> Hi,
> 
> On Wed, Sep 26, 2018 at 06:17:34AM -0400, Frediano Ziglio wrote:
> > > 
> > > From: Victor Toso <me at victortoso.com>
> > > 
> > > 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 at redhat.com>
> > >     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 at redhat.com>
> > > ---
> > >  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 at redhat.com>
> > 
> > Frediano
> 
> Thanks,
> 
>