On Tue, 2018-06-05 at 19:26 +0200, Marc-André Lureau wrote:
> Hi
>
> On Thu, May 31, 2018 at 8:35 PM, Jonathon Jongsma <jjongsma@xxxxxxxxx
> m> 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.
>
> I think I understand the issue, but is there a bug report about it?
> If
> yes, could you include it in the commit message?
There's no bug report as far as I know, but there was a patch from Qiu
Wenbo to the mailing list a while back reverting 9fbf6794. Rather than
reverting that patch, I decided to try to to fix it a different way.
>
> What if the application crashes? Shouldn't we have a strong mechanism
> to release the USB devices then?
Yes, I suppose it would be a good idea.
>
> >
> > 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.
>
> That seems reasonable to me.
>
> > ---
> > 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.
>
> missing Since: tag
>
> > + **/
> > + signals[SPICE_SESSION_DISCONNECTED] =
> > + g_signal_new("disconnected",
> > + 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);
>
> Instead of introducing a new list, why not move the ring_remove() of
> channel here and check it is empty?
>
> > + 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);
> > }
> >
> > 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);
> >
> > /*< private >*/
> > /*
> > --
> > 2.14.3
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel