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

[Victor Toso]

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.

Well, all might be fixed with the recent rework there with cd
redir...  More things to understand.

> 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?

> 
> > +        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,

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180926/ed6dddff/attachment.sig>