On 9/6/18 11:13 AM, Victor Toso wrote: > From: Victor Toso <me@xxxxxxxxxxxxxx> > > 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. > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1625550 > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx> > --- > 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. Does it really need to be called from that function? > } > > 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. -- Eduardo de Barros Lima (Etrunko) Software Engineer - RedHat etrunko@xxxxxxxxxx _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel