[Spice-devel] [spice-server] playback: Don't lose client connection after migration

[Jonathon Jongsma]

On Wed, 2017-04-19 at 05:30 -0400, Frediano Ziglio wrote:
> Merged.
> 
> Would be great to have some lower level (lower than having to do a
> migration) test
> (like unit testing) of this stuff.

agreed

> 
> I was thinking (time ago) of removing the connection field in favour
> of
> also supporting multiple clients too (at least for playback).
> 
> Is there a reason to have separate on_new_* functions? They are
> called just from constructors.

I suspect it was just because we were trying to minimize code churn
when refactoring. But perhaps it makes sense to do some additional
refactoring here. I'll send some patches.

> 
> I noted that these on_new_* functions use client->active however
> looking
> at code it seems that this field is initialized only later basically
> having a dead code. Is it really dead code? Were these lines always
> dead code?

I'll look into it.

> 
> Frediano
> 
> > 
> > Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
> > 
> > 
> > On Fri, 2017-04-14 at 09:02 +0200, Christophe Fergeau wrote:
> > > Commit 590acf3c556 introduced a regression after migration:
> > > PlaybackChannel::connection is never set even if a client is
> > > connected,
> > > which means the playback channel is non-functional until a client
> > > reconnects as all the snd_channel_set_xxx() method checks for a
> > > non-
> > > NULL
> > > PlaybackChannel::connection before sending data to the client.
> > > 
> > > This commit slightly changes the code flow in
> > > on_new_playback_channel_client()/playback_channel_client_construc
> > > ted(
> > > )
> > > so that PlaybackChannel::connection is set regardless of what
> > > red_client_during_migrate_at_target() returns. This is what was
> > > done
> > > prior to 590acf3c556.
> > > 
> > > This resolves https://bugs.freedesktop.org/show_bug.cgi?id=100136
> > > 
> > > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > > ---
> > >  server/sound.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/server/sound.c b/server/sound.c
> > > index 6a6d965..b50f9fc 100644
> > > --- a/server/sound.c
> > > +++ b/server/sound.c
> > > @@ -991,10 +991,14 @@ static int snd_desired_audio_mode(bool
> > > playback_compression, int frequency,
> > >  static void on_new_playback_channel_client(SndChannel *channel,
> > > SndChannelClient *client)
> > >  {
> > >      RedsState *reds =
> > > red_channel_get_server(RED_CHANNEL(channel));
> > > +    RedClient *red_client =
> > > red_channel_client_get_client(RED_CHANNEL_CLIENT(client));
> > >  
> > >      spice_assert(client);
> > >  
> > >      channel->connection = client;
> > > +    if (red_client_during_migrate_at_target(red_client)) {
> > > +        return;
> > > +    }
> > >      snd_set_command(client, SND_PLAYBACK_MODE_MASK);
> > >      if (client->active) {
> > >          snd_set_command(client, SND_CTRL_MASK);
> > > @@ -1036,7 +1040,6 @@ playback_channel_client_constructed(GObject
> > > *object)
> > >  {
> > >      PlaybackChannelClient *playback_client =
> > > PLAYBACK_CHANNEL_CLIENT(object);
> > >      RedChannel *red_channel =
> > > red_channel_client_get_channel(RED_CHANNEL_CLIENT(playback_client
> > > ));
> > > -    RedClient *client =
> > > red_channel_client_get_client(RED_CHANNEL_CLIENT(playback_client)
> > > );
> > >      SndChannel *channel = SND_CHANNEL(red_channel);
> > >  
> > >      G_OBJECT_CLASS(playback_channel_client_parent_class)-
> > > > constructed(object);
> > > 
> > > @@ -1061,9 +1064,7 @@ playback_channel_client_constructed(GObject
> > > *object)
> > >          }
> > >      }
> > >  
> > > -    if (!red_client_during_migrate_at_target(client)) {
> > > -        on_new_playback_channel_client(channel,
> > > SND_CHANNEL_CLIENT(playback_client));
> > > -    }
> > > +    on_new_playback_channel_client(channel,
> > > SND_CHANNEL_CLIENT(playback_client));
> > >  
> > >      if (channel->active) {
> > >          snd_playback_start(channel);