> > > > > 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