On Thu, 2018-05-31 at 16:41 -0400, Frediano Ziglio 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.
> > ---
> > src/spice-session.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > src/spice-session.h | 2 ++
> > 2 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index 57acc63..de1128a 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;
> > + GList *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,
> > };
> >
> > @@ -1313,6 +1315,23 @@ 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.
> > + **/
> > + signals[SPICE_SESSION_DISCONNECTED] =
> > + g_signal_new("disconnected",
>
> why not "session-disconnected" ?
Because the object is SpiceSession. So you would expect
SpiceSession::disconnected to mean that the session has been
disconnected. The only reason that the other signal is called
SpiceSession::channel-destroy is because it's not referring to the
session, but the channel instead.
>
> > + G_OBJECT_CLASS_TYPE(gobject_class),
> > + G_SIGNAL_RUN_FIRST,
> > + G_STRUCT_OFFSET(SpiceSessionClass,
> > disconnected),
> > + 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 = g_list_remove(s->channels_destroying,
> > channel);
>
> So you insert before adding the weak reference and you just remove
> when
> the object is freed checking when the list is empty. Why not using a
> simple counter checking when is 0 ?
I could, I suppose. It would certainly be simpler. Keeping them in a
list gives us a little bit of potential debugging help. For example, if
not all channels were freed, we could check which channel was in the
channels_destroying list and preventing us from emitting the signal.
Not sure that it would be useful very often though, so I don't feel
strongly either way.
>
> > + if (!s->channels_destroying) {
> > + 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));
> > @@ -2298,10 +2328,17 @@ static void
> > spice_session_channel_destroy(SpiceSession *session, SpiceChannel
> > *c
> > ring_remove(&item->link);
> > g_free(item);
> >
> > - g_signal_emit(session, signals[SPICE_SESSION_CHANNEL_DESTROY],
> > 0,
> > channel);
> >
> > g_clear_object(&channel->priv->session);
> > spice_channel_disconnect(channel, SPICE_CHANNEL_NONE);
> > +
> > + g_signal_emit(session, signals[SPICE_SESSION_CHANNEL_DESTROY],
> > 0,
> > channel);
> > +
> > + /* Wait until the channel is properly freed so that we can
> > emit a
> > + * 'disconnected' signal */
> > + s->channels_destroying = g_list_prepend(s-
> > >channels_destroying,
> > channel);
> > + g_object_weak_ref(G_OBJECT(channel),
> > channel_finally_destroyed,
> > g_object_ref(session));
> > +
> > g_object_unref(channel);
>
> isn't it possible that the channel reference reach 0 before we
> enlisted/counted
> all channels so we will think that all channels were freed?
Not quite sure I understand the question.
>
> > }
> >
> > diff --git a/src/spice-session.h b/src/spice-session.h
> > index cfe02b1..4a2f0ce 100644
> > --- a/src/spice-session.h
> > +++ b/src/spice-session.h
> > @@ -84,6 +84,7 @@ struct _SpiceSession
> > * @parent_class: Parent class.
> > * @channel_new: Signal class handler for the
> > #SpiceSession::channel_new
> > signal.
> > * @channel_destroy: Signal class handler for the
> > #SpiceSession::channel_destroy signal.
> > + * @disconnected: Signal class handler for the
> > #SpiceSession::disconnected
> > signal.
> > *
> > * Class structure for #SpiceSession.
> > */
> > @@ -94,6 +95,7 @@ struct _SpiceSessionClass
> > /* signals */
> > void (*channel_new)(SpiceSession *session, SpiceChannel
> > *channel);
> > void (*channel_destroy)(SpiceSession *session, SpiceChannel
> > *channel);
> > + void (*disconnected)(SpiceSession *session);
> >
>
> Do we really think this ABI change?
> If yes we need to bump the version.
No. I was just thinking about that after sending this patch. I will
remove this ABI change.
>
> > /*< private >*/
> > /*
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel