[Spice-devel] [spice-server] sound: Don't mute recording when client reconnects

[Frediano Ziglio]

> 
> On Wed, May 23, 2018 at 01:36:35PM -0400, Frediano Ziglio wrote:
> > Testing this, so far is working correctly:
> > 
> > Subject: [PATCH spice-server] rcc: Make red_channel_client_is_connected
> > return
> >  correct information during initialization
> > 
> > Use stream->watch as flag to check if connected or not initializing
> > it during construction, not during init (after all hierarchy is built).
> 
> 
> If there are any failures during red_channel_client_initable_init(),
> red_channel_client_is_connected() will still return TRUE with this
> patch.
> 
> In general, we should consider RedChannelClient and its derivative to be
> non-functional until red_channel_client_initable_init() successfully
> returns. This change makes RedChannelClient::is_connected become TRUE
> much earlier in the instantiation process, imo we should not make the
> object "look ready" until red_channel_client_initable_init() has
> returned, I'm worried we'd end up doing some eg network operations
> because the socket looks ready, while some other important things for
> the channel haven't been initialized yet. SndChannelClient is the only
> class trying to do network operations, or 'complex' stuff as part of its
> instantiation btw, maybe I should move more of it to _initable_init.
> 
> With that said, and apart from that "early init" issue, the patch
> makes sense.
> 
> Christophe
> 

Surely the "red_channel_client_is_connected" name and current behaviour
are wrong. If the check is for readiness, then should be renamed to
red_channel_client_is_ready, the channel in its initial state is already
"connected" as there is a client with a socket connected even before
g_object_new is even called.

Another question can be: "Is red_channel_client_is_connected called
to check connection or that is it into the list?". The only occurrence
that actually is not related to just connection is the call inside
red_channel_client_disconnect. Note that the watch is removed on
red_channel_client_shutdown and red_channel_client_disconnect calls,
first is forcing disconnection while second is usually handling it.
I'll send an update to do the proper check in red_channel_client_disconnect.

OT, but not too much. Is red_channel_client_shutdown doing the
right thing? Is shutting down the socket but also removing the
watch which is supposed to trigger the client disconnection.
So if the watch is removed how the code will handle the disconnection?
red_channel_client_shutdown is not called that often, some uncommon
error in agent/vdi/char devices. Is this path of the code even
tested?

Frediano

> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-channel-client.c | 33 ++++++++++++++++++++++-----------
> >  1 file changed, 22 insertions(+), 11 deletions(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> > index 893a764d..0057108f 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -173,6 +173,7 @@ static void
> > red_channel_client_clear_sent_item(RedChannelClient *rcc);
> >  static void red_channel_client_initable_interface_init(GInitableIface
> >  *iface);
> >  static void red_channel_client_set_message_serial(RedChannelClient
> >  *channel, uint64_t);
> >  static bool red_channel_client_config_socket(RedChannelClient *rcc);
> > +static void red_channel_client_event(int fd, int event, void *data);
> >  
> >  /*
> >   * When an error occurs over a channel, we treat it as a warning
> > @@ -293,6 +294,22 @@ red_channel_client_get_property(GObject *object,
> >      }
> >  }
> >  
> > +static void
> > +red_channel_client_init_watch(RedChannelClient *rcc)
> > +{
> > +    if (!rcc->priv->stream || rcc->priv->stream->watch ||
> > !rcc->priv->channel) {
> > +        return;
> > +    }
> > +
> > +    SpiceCoreInterfaceInternal *core =
> > red_channel_get_core_interface(rcc->priv->channel);
> > +    red_stream_set_core_interface(rcc->priv->stream, core);
> > +    rcc->priv->stream->watch =
> > +        core->watch_add(core, rcc->priv->stream->socket,
> > +                        SPICE_WATCH_EVENT_READ,
> > +                        red_channel_client_event,
> > +                        rcc);
> > +}
> > +
> >  static void
> >  red_channel_client_set_property(GObject *object,
> >                                  guint property_id,
> > @@ -305,11 +322,13 @@ red_channel_client_set_property(GObject *object,
> >      {
> >          case PROP_STREAM:
> >              self->priv->stream = g_value_get_pointer(value);
> > +            red_channel_client_init_watch(self);
> >              break;
> >          case PROP_CHANNEL:
> >              if (self->priv->channel)
> >                  g_object_unref(self->priv->channel);
> >              self->priv->channel = g_value_dup_object(value);
> > +            red_channel_client_init_watch(self);
> >              break;
> >          case PROP_CLIENT:
> >              self->priv->client = g_value_get_object(value);
> > @@ -913,7 +932,6 @@ static gboolean
> > red_channel_client_initable_init(GInitable *initable,
> >                                                   GError **error)
> >  {
> >      GError *local_error = NULL;
> > -    SpiceCoreInterfaceInternal *core;
> >      RedChannelClient *self = RED_CHANNEL_CLIENT(initable);
> >  
> >      if (!self->priv->stream) {
> > @@ -932,16 +950,10 @@ static gboolean
> > red_channel_client_initable_init(GInitable *initable,
> >          goto cleanup;
> >      }
> >  
> > -    core = red_channel_get_core_interface(self->priv->channel);
> > -    red_stream_set_core_interface(self->priv->stream, core);
> > -    self->priv->stream->watch =
> > -        core->watch_add(core, self->priv->stream->socket,
> > -                        SPICE_WATCH_EVENT_READ,
> > -                        red_channel_client_event,
> > -                        self);
> > -
> >      if (self->priv->monitor_latency
> >          && red_stream_get_family(self->priv->stream) != AF_UNIX) {
> > +        SpiceCoreInterfaceInternal *core =
> > red_channel_get_core_interface(self->priv->channel);
> > +
> >          self->priv->latency_monitor.timer =
> >              core->timer_add(core, red_channel_client_ping_timer, self);
> >  
> > @@ -1665,8 +1677,7 @@ bool
> > red_channel_client_is_mini_header(RedChannelClient *rcc)
> >  
> >  gboolean red_channel_client_is_connected(RedChannelClient *rcc)
> >  {
> > -    return rcc->priv->channel
> > -        && (g_list_find(red_channel_get_clients(rcc->priv->channel), rcc)
> > != NULL);
> > +    return rcc->priv->stream->watch != NULL;
> >  }
> >  
> >  static void red_channel_client_clear_sent_item(RedChannelClient *rcc)