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

[Victor Toso]

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!

> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171205/c9a61859/attachment-0001.sig>