[Spice-devel] [spice-server v2] sound: do not change volume or mute state on migration

[Frediano Ziglio]

> 
> Hi,
> 
> On Tue, Dec 05, 2017 at 03:58:56AM -0500, Frediano Ziglio wrote:
> > > 
> > > From: Victor Toso <me at victortoso.com>
> > > 
> > > On migration, Qemu notify spice-server with the current Guest volume
> > > and mute state values which currently is handled forwarding these
> > > values to the client.
> > > 
> > > This patch is a complement of f10de4bc084fcc - Here, volume was
> > > jumping regardless of guest's volume value.
> > > 
> > > Resolves: rhbz#1425443
> > > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > > ---
> > >  server/sound.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/server/sound.c b/server/sound.c
> > > index b1bfaaaa..fc3d8f4a 100644
> > > --- a/server/sound.c
> > > +++ b/server/sound.c
> > > @@ -823,6 +823,7 @@ static void snd_channel_set_volume(SndChannel
> > > *channel,
> > >  {
> > >      SpiceVolumeState *st = &channel->volume;
> > >      SndChannelClient *client = snd_channel_get_client(channel);
> > > +    RedChannelClient *rcc;
> > >  
> > >      st->volume_nchannels = nchannels;
> > >      g_free(st->volume);
> > > @@ -831,6 +832,10 @@ static void snd_channel_set_volume(SndChannel
> > > *channel,
> > >      if (!client || nchannels == 0)
> > >          return;
> > >  
> > > +    rcc = RED_CHANNEL_CLIENT(client);
> > > +    if
> > > (red_client_during_migrate_at_target(red_channel_client_get_client(rcc)))
> > > +        return;
> > > +
> > >      snd_set_command(client, SND_VOLUME_MASK);
> > >      snd_send(client);
> > >  }
> > > @@ -846,12 +851,17 @@ static void snd_channel_set_mute(SndChannel
> > > *channel,
> > > uint8_t mute)
> > >  {
> > >      SpiceVolumeState *st = &channel->volume;
> > >      SndChannelClient *client = snd_channel_get_client(channel);
> > > +    RedChannelClient *rcc;
> > >  
> > >      st->mute = mute;
> > >  
> > >      if (!client)
> > >          return;
> > >  
> > > +    rcc = RED_CHANNEL_CLIENT(client);
> > > +    if
> > > (red_client_during_migrate_at_target(red_channel_client_get_client(rcc)))
> > > +        return;
> > > +
> > >      snd_set_command(client, SND_MUTE_MASK);
> > >      snd_send(client);
> > >  }
> > 
> > I'll test today.
> 
> Thanks, I hope to test it after lunch here.
> 
> > As a minor style (I can fix, no issues), we always wants brackets for if
> > statement, even for a single "return" line.
> 
> Ok!
> 

So, as we were discussing on irc I did some checks and got a reproduction
however the issue is not in the server but in the client.
The client is sending sometimes the connection_id (link message) as 0 telling
the server that this is a new connection so the server correctly send the
mute/volume information.
Qemu is correctly telling spice the mute/volume on the new instance before
any connection is established so the code above does nothing as the check
for client make this new code unreachable (unless you set mute/volume later,
in this case you want it send it to the client).

> > OT: Looking at this code and considering other channels why we should
> > care if RedClient or RedChannel is migrating and what does it means that
> > a RedChannel is migrating? If RedClient is migrating means there are some
> > channels that have to be migrated. In this case won't make more sense to
> > ask if this specific RedChannelClient is still migrating?
> > In case of RedChannel (like
> > common_graphics_channel_get_during_target_migrate)
> > why this should be common to all related RedChannelClient? If I have a
> > client
> > migrating and another just connected should not the behaviour be different
> > on the 2 clients? I know, we don't support multiple client but still looks
> > in theory wrong to me.
> 
> I followed the design of other parts of the code but I'm not really sure
> why it is this way. Migration works quite well with me, not sure if we
> deal that with multiple client.
> 
> Actually, does the multiple client still works after the code refactor?
> (SPICE_DEBUG_ALLOW_MC_ENV=1) it's been awhile since I tested it.
> 

I think migration and multiple clients never worked. But this is not
a good reason to think in the right way. Is always confusing to follow
software that are not doing the right things.

Frediano