>
> On Tue, Jun 5, 2018 at 10:40 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> wrote:
> > Previously, an application was required to determine whether a session
> > had been disconnected by listening for the SpiceSession::channel-destroy
> > signal for each individual channel. The Application had to keep track of
> > whether all channels had been destroyed.
> >
> > An additional difficulty is that when a SpiceSession::channel-destroy
> > signal is emitted, the channel object has not been freed yet, so any
> > shutdown/de-allocation actions have not yet happened. This became
> > significant since virt-viewer exits the application in response to the
> > last 'channel-destroy' signal, which means that the channel objects are
> > never properly freed. This is particularly problematic for the usbredir
> > channel, which might need to disconnect USB devices asynchronously, and
> > if the application exits before that process has completed, the USB
> > devices may not be available on the client machine.
> >
> > With this patch, the SpiceSession still emits the 'channel-destroy'
> > signal for each channel when it is disconnected. But it also internally
> > tracks which channels have been destroyed, and waits for them to be
> > freed. When all channels are freed, it emits an additional
> > 'disconnected' signal. An application can be confident that all
> > channels have been freed by this point, and the application can exit
> > safely.
> > ---
> > Changes in v2:
> > - remove ABI change ('disconnected' vfunc in SpiceSession struct
> > - use an integer to track disconnecting channels instead of a list of
> > objects
>
> "why not move the ring_remove() of
> channel here and check it is empty?"
>
I think would be potentially a regression.
The ring is used to scan the channels and potentially perform some
operations on them which could be potentially unsafe is the channel
is being destroyed.
> > - Add a warning to spice_session_dispose() if there are channels
> > disconnecting when the session is destroyed.
> >
> > src/spice-session.c | 36 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 36 insertions(+)
> >
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index 57acc63..1731904 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -94,6 +94,7 @@ struct _SpiceSessionPrivate {
> > int protocol;
> > SpiceChannel *cmain; /* weak reference */
> > Ring channels;
> > + guint channels_destroying;
> > guint32 mm_time;
> > gboolean client_provided_sockets;
> > guint64 mm_time_at_clock;
> > @@ -210,6 +211,7 @@ enum {
> > SPICE_SESSION_CHANNEL_NEW,
> > SPICE_SESSION_CHANNEL_DESTROY,
> > SPICE_SESSION_MM_TIME_RESET,
> > + SPICE_SESSION_DISCONNECTED,
> > SPICE_SESSION_LAST_SIGNAL,
> > };
> >
> > @@ -343,6 +345,7 @@ spice_session_dispose(GObject *gobject)
> > g_warn_if_fail(s->migration_left == NULL);
> > g_warn_if_fail(s->after_main_init == 0);
> > g_warn_if_fail(s->disconnecting == 0);
> > + g_warn_if_fail(s->channels_destroying == 0);
> >
> > g_clear_object(&s->audio_manager);
> > g_clear_object(&s->usb_manager);
> > @@ -1313,6 +1316,22 @@ static void
> > spice_session_class_init(SpiceSessionClass *klass)
> > 1,
> > SPICE_TYPE_CHANNEL);
> >
> > + /**
> > + * SpiceSession::disconnected:
> > + * @session: the session that emitted the signal
> > + *
> > + * The #SpiceSession::disconnected signal is emitted when all channels
> > have been destroyed.
> > + **/
>
> missing since tag
>
> > + signals[SPICE_SESSION_DISCONNECTED] =
> > + g_signal_new("disconnected",
> > + G_OBJECT_CLASS_TYPE(gobject_class),
> > + G_SIGNAL_RUN_FIRST,
> > + 0, NULL, NULL,
> > + g_cclosure_marshal_VOID__VOID,
> > + G_TYPE_NONE,
> > + 0,
> > + NULL);
> > +
> > /**
> > * SpiceSession::mm-time-reset:
> > * @session: the session that emitted the signal
> > @@ -2269,6 +2288,17 @@ void spice_session_channel_new(SpiceSession
> > *session, SpiceChannel *channel)
> > g_signal_emit(session, signals[SPICE_SESSION_CHANNEL_NEW], 0,
> > channel);
> > }
> >
> > +static void channel_finally_destroyed(gpointer data, GObject *channel)
> > +{
> > + SpiceSession *session = SPICE_SESSION(data);
> > + SpiceSessionPrivate *s = session->priv;
> > + s->channels_destroying--;
> > + if (s->channels_destroying == 0) {
> > + g_signal_emit(session, signals[SPICE_SESSION_DISCONNECTED], 0);
> > + }
> > + g_object_unref(session);
> > +}
> > +
> > static void spice_session_channel_destroy(SpiceSession *session,
> > SpiceChannel *channel)
> > {
> > g_return_if_fail(SPICE_IS_SESSION(session));
> > @@ -2302,6 +2332,12 @@ static void
> > spice_session_channel_destroy(SpiceSession *session, SpiceChannel *c
> >
> > g_clear_object(&channel->priv->session);
> > spice_channel_disconnect(channel, SPICE_CHANNEL_NONE);
> > +
> > + /* Wait until the channel is properly freed so that we can emit a
> > + * 'disconnected' signal */
> > + s->channels_destroying++;
> > + g_object_weak_ref(G_OBJECT(channel), channel_finally_destroyed,
> > g_object_ref(session));
> > +
> > g_object_unref(channel);
> > }
> >
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel