[Spice-devel] [spice-gtk v1 4/4] channel-usbredir: Chain-up with parent's channel-reset

[Victor Toso]

Hi,

On Tue, Sep 25, 2018 at 02:12:14PM -0300, Eduardo Lima (Etrunko) wrote:
> On 9/6/18 11:13 AM, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> > 
> > Otherwise spice-channel is left with a broken state.
> > 
> > This code moves parent's call to channel_reset() into
> > _channel_reset_finish() - Note that spice-channel's channel_reset()
> > can be called from GMainContext.
> > 
> 
> I think it would be easier to review if 3 and 4 were squashed
> together.

I don't agree because while reading this code, I found out an
important different problem that I tried to document on 3/4
patch while this one fixes the problem around rhbz#1625550

> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1625550
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/channel-usbredir.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > index 7f6e2e3..96c636f 100644
> > --- a/src/channel-usbredir.c
> > +++ b/src/channel-usbredir.c
> > @@ -122,7 +122,7 @@ static void spice_usbredir_channel_init(SpiceUsbredirChannel *channel)
> >  
> >  #ifdef USE_USBREDIR
> >  
> > -static void _channel_reset_finish(SpiceUsbredirChannel *channel)
> > +static void _channel_reset_finish(SpiceUsbredirChannel *channel, gboolean migrating)
> >  {
> >      SpiceUsbredirChannelPrivate *priv = channel->priv;
> >  
> > @@ -135,6 +135,8 @@ static void _channel_reset_finish(SpiceUsbredirChannel *channel)
> >      spice_usbredir_channel_set_context(channel, priv->context);
> >  
> >      spice_usbredir_channel_unlock(channel);
> > +
> > +    SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)->channel_reset(SPICE_CHANNEL(channel), migrating);
> 
> I noticed that you are chaining up channel_reset() indirectly
> from the callback, which does not seem quite right to me.

The 3/4 patch added some context for this. There is scenario
where this can happen async, when priv->state == STATE_CONNECTED
on spice_usbredir_channel_reset(); This code path is a bit wrong
as channel-reset is expected to be sync, not async. Fully agree
with Frediano at

    https://lists.freedesktop.org/archives/spice-devel/2018-September/045549.html

But this patch is not touching this problem, it is fixing not
calling parent's channel-reset in another scenario.

> Does it really need to be called from that function?

Either that or duplicate some other function calls from
_channel_reset_finish() here. I opted for the first option.

> >  }
> >  
> >  static void _channel_reset_cb(GObject *gobject,
> > @@ -146,9 +148,7 @@ static void _channel_reset_cb(GObject *gobject,
> >      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);
> > +    _channel_reset_finish(channel, migrating);
> >  
> >      spice_usbredir_channel_disconnect_device_finish(channel, result, &err);
> >  }
> > @@ -177,8 +177,7 @@ static void spice_usbredir_channel_reset(SpiceChannel *c, gboolean migrating)
> >          return;
> >      }
> >  
> > -    /* FIXME: This does not chain-up with parent's channel-reset, which is a must */
> > -    _channel_reset_finish(channel);
> > +    _channel_reset_finish(channel, migrating);
> 
> Instead of passing the migrating parameter, it looks more
> appropriate to call the parent class function at this point, as
> a matter of consistency with the name of the function.

Thanks for review,
Victor

> 
> -- 
> Eduardo de Barros Lima (Etrunko)
> Software Engineer - RedHat
> etrunko at redhat.com
-------------- 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/1c4d5052/attachment-0001.sig>