Hi,
Just to mention that I tested both patches and they work as
expected in the situation around rhbz#1549132
toso
On Wed, May 23, 2018 at 01:36:35PM -0400, Frediano Ziglio wrote:
> > > When a new record channel is added, the code relies on a
> > > snd_send() call in record_channel_client_constructed() to
> > > send RECORD_START to the client. However, at this point,
> > > snd_send() is non-functional because the
> > > red_channel_client_pipe_add() call it makes is a no-op
> > > because prepare_pipe_add() makes a connection check through
> > > red_channel_client_is_connected() queueing the item. This
> > > connection
> >
> > The patch looks surely good, but why red_channel_client_is_connected
> > returns a wrong value? The channel is connected but asking if is
> > connected returns false... Is currently implemented as: is in the
> > channel list so is connected, otherwise is not which during construction
> > is wrong as still not in the list.
> > Won't be better to check if really connected?
> >
> > > check returns FALSE at ::constructed() time as the channel client will
> > > only become connected towards the end of
> > > red_channel_client_initable_init() which runs after the object
> > > instantiation is complete.
> > >
> >
> > For me a RCC is connected if there is a client connected at socket
> > level, being into a list of RCCs is not a really a great definition
> > for connected.
> >
> > Maybe removing the check for red_channel_client_is_connected on
> > prepare_pipe_add would solve the issue?
> >
> > > This causes a bug where starting recording and then
> > > disconnecting/reconnecting the client does not successfully reenable
> > > recording. This is a regression introduced by commit d8dc09
> > > 'sound: Convert SndChannelClient to RedChannelClient'
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1549132
> > >
> > > Signed-off-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> > > ---
> > > server/sound.c | 37 ++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 34 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/server/sound.c b/server/sound.c
> > > index e3891d2cc..d461b9272 100644
> > > --- a/server/sound.c
> > > +++ b/server/sound.c
> > > @@ -105,7 +105,11 @@ typedef struct SndChannelClientClass {
> > > RedChannelClientClass parent_class;
> > > } SndChannelClientClass;
> > >
> > > -G_DEFINE_TYPE(SndChannelClient, snd_channel_client,
> > > RED_TYPE_CHANNEL_CLIENT)
> > > +static void snd_channel_client_initable_interface_init(GInitableIface
> > > *iface);
> > > +static GInitableIface *snd_channel_client_parent_initable_iface;
> > > +
> > > +G_DEFINE_TYPE_WITH_CODE(SndChannelClient, snd_channel_client,
> > > RED_TYPE_CHANNEL_CLIENT,
> > > + G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE,
> > > snd_channel_client_initable_interface_init))
> > >
> > >
> > > enum {
> > > @@ -1083,7 +1087,6 @@ playback_channel_client_constructed(GObject *object)
> > > if (channel->active) {
> > > playback_channel_client_start(scc);
> > > }
> > > - snd_send(scc);
> > > }
> > >
> > > static void snd_set_peer(RedChannel *red_channel, RedClient *client,
> > > RedStream *stream,
> > > @@ -1269,7 +1272,6 @@ record_channel_client_constructed(GObject *object)
> > > if (channel->active) {
> > > record_channel_client_start(scc);
> > > }
> > > - snd_send(scc);
> > > }
> > >
> > >
> > > @@ -1480,6 +1482,35 @@ snd_channel_client_init(SndChannelClient *self)
> > > {
> > > }
> > >
> > > +static gboolean snd_channel_client_initable_init(GInitable *initable,
> > > + GCancellable
> > > *cancellable,
> > > + GError **error)
> > > +{
> > > + gboolean success;
> > > +
> > > + success = snd_channel_client_parent_initable_iface->init(initable,
> > > cancellable, error);
> > > + if (!success) {
> > > + return FALSE;
> > > + }
> > > + /* Wait until the very end of the initialization as the channel client
> > > + * becomes connected (red_channel_client_is_connected() starts
> > > returning
> > > + * TRUE) only very late in red_channel_client_initable_init()
> > > + * Before that, red_channel_client_pipe_add() is a no-op because a
> > > connection check is done in
> > > + * prepare_pipe_add()
> > > + */
> > > + snd_send(SND_CHANNEL_CLIENT(initable));
> > > +
> > > + return TRUE;
> > > +}
> > > +
> > > +static void snd_channel_client_initable_interface_init(GInitableIface
> > > *iface)
> > > +{
> > > + snd_channel_client_parent_initable_iface =
> > > g_type_interface_peek_parent
> > > (iface);
> > > +
> > > + iface->init = snd_channel_client_initable_init;
> > > +}
> > > +
> > > +
> > > static void
> > > playback_channel_client_class_init(PlaybackChannelClientClass *klass)
> > > {
> >
>
> 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).
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> 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)
> --
>
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel