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@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_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@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