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

[Eduardo Lima (Etrunko)]

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.

> 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. 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 at redhat.com