Merged. Would be great to have some lower level (lower than having to do a migration) test (like unit testing) of this stuff. 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 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? Frediano > > Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > > > 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_constructed( > > ) > > 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@xxxxxxxxxx> > > --- > > 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); _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel