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@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. 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@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. 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@xxxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel