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

Submitted by Victor Toso on Dec. 4, 2017, 8:09 a.m.

Details

Message ID 20171204080922.5792-1-victortoso@redhat.com
State New
Headers show
Series "sound: do not change volume or mute state on migration" ( rev: 3 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Dec. 4, 2017, 8:09 a.m.
From: Victor Toso <me@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@redhat.com>
---
 server/sound.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Patch hide | download patch | download mbox

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);
 }

Comments

Hi,

On Mon, Dec 04, 2017 at 09:09:22AM +0100, Victor Toso wrote:
> From: Victor Toso <me@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@redhat.com>

JFYI, did not test this version just yet.

The infra that I had for playing around with migration is not working
at the moment, so I'm setting up everything in another machine to test
this soon. I don't see why it wouldn't work but better safe than sorry.

Cheers,

> ---
>  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);
>  }
> -- 
> 2.15.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> Hi,
> 
> On Mon, Dec 04, 2017 at 09:09:22AM +0100, Victor Toso wrote:
> > From: Victor Toso <me@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@redhat.com>
> 
> JFYI, did not test this version just yet.
> 
> The infra that I had for playing around with migration is not working
> at the moment, so I'm setting up everything in another machine to test
> this soon. I don't see why it wouldn't work but better safe than sorry.
> 
> Cheers,
> 

Would be great to have a test in spice-server too but looks like lot
of blocks are missing for this.
I have a working environment, I'll have a try (at least testing this
patch).

Frediano

> > ---
> >  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);
> >  }
> 
> From: Victor Toso <me@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@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.
As a minor style (I can fix, no issues), we always wants brackets for if
statement, even for a single "return" line.

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.

Frediano
Hi,

On Tue, Dec 05, 2017 at 03:58:56AM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me@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@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.
> 
> Hi,
> 
> On Tue, Dec 05, 2017 at 03:58:56AM -0500, Frediano Ziglio wrote:
> > > 
> > > From: Victor Toso <me@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@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