[Spice-devel] [RFC spice-vdagent 00/18] GLib integration

[Jakub Janku]

Hi,

On Tue, Sep 4, 2018 at 7:33 AM Victor Toso <victortoso at redhat.com> wrote:
>
> Hi,
>
> On Mon, Sep 03, 2018 at 09:03:38PM +0200, Jakub Janku wrote:
> > I do agree that turning VDAgentConnection into a GObject is
> > probably a good idea, however, I do not see how it could help
> > us in this situation. Am I missing something?  I'll try to
> > explain the problem again:
> >
> > Let's consider that vdagentd receives a signal.
> > signal_handler() in vdagentd.c is called which quits the main
> > loop.  The current loop iteration is finished and the program
> > returns to main().  In main(), vdagent_virtio_port_destroy() is
> > called, which internally invokes vdagent_connection_destroy().
>
> That is:
>
>  | static gboolean signal_handler(gpointer user_data)
>  | {
>  |     g_main_loop_quit(loop);
>  |     return G_SOURCE_REMOVE;
>  | }
>
> But note that g_main_loop_quit() docs says that:
>
>  | Note that sources that have already been dispatched when
>  | g_main_loop_quit() is called will still be executed.
>
> So, isn't this a matter of having cancelled everything correctly
> at the right time?

Well, it kinda is. I think that the call to g_main_loop_quit() should
be made once the connection_finalize() in vdagent-connection.c is
called.
The question is what's the best way to accomplish this.
>
> > If the virtio port was connected at the moment we received the
> > signal, there was also a running GTask which was previously
> > started with g_input_stream_read_all_async() (because we try to
> > read from the port the whole time).
> > This GTask holds a reference to the GIOStream, this means that
> > the stream won't be closed until the GTask finishes.
> > However, the GTask cannot be canceled synchronously (see docs
> > for g_cancellable_cancel()) and the GMainLoop is NOT running at
> > the moment.
>
> It is fine that it isn't sync. What happens is that, it gets
> cancelled (because g_main_loop_quit()) and on GIOStream's
> callback, you would get the GError::G_IO_ERROR_CANCELLED, that
> callback would return and that GSource finished.
>
> When the last GSource is finished is when g_main_loop_run()
> returns and main() is executed again.
>
> > As a result, the reference to GIOStream is not released and the
> > FD is not closed.
> >
> > So the problem aren't the references to VDAgentConnection
> > itself, but rather the GLib's internal references to the
> > GIOStream we use in VDAgentConnection.
>
> If we follow glib's API we either should be fine or find a bug in
> glib

It's not that simple I'm afraid.
Have a look at https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html
and the diagram (
https://developer.gnome.org/glib/stable/mainloop-states.gif )
When signal_handler() is called, the main loop is in the "dispatching"
phase. We can call vdagent_virtio_port_destroy() in the signal
handler, but the problem is that if we call g_main_loop_quit(), the
main loop won't iterate again. And we need that extra iterations
because GCancellable uses file descriptor and hence the main loop
needs to go through that "polling" phase so that we get the
GError::G_IO_ERROR_CANCELLED error.

So in other words, if the loop is quit too early, the GIOStream's
callback won't be called and connection won't be finalized.

Cheers,
Jakub

>
> > I hope this makes it a bit more clear.
>
> Let me know if I get it wrong again ;)
>
> Cheers,
> Victor
>
> > Thanks for the reviews :)
> > Jakub
> >
> > > >
> > > > Jakub Janků (18):
> > > >   vdagentd: parse argv using GLib
> > > >   vport: add by_user param to vdagent_virtio_port_disconnect_callback
> > > >   vdagentd: use GMainLoop
> > > >   build: add GIO dependency
> > > >   add VDAgentConnection
> > > >   udscs: add udscs_get_peer_pid()
> > > >   udscs: use VDAgentConnection
> > > >   udscs-server: split initialization
> > > >   udscs: simplify logging
> > > >   vport: use VDAgentConnection
> > > >   session-info: add ActiveSessionChangeCb
> > > >   console-kit: use GDBus
> > > >   systemd-login: use GDBus
> > > >   session-info: remove session_info_get_fd()
> > > >   build: drop DBus dependency
> > > >   move to GLib memory functions
> > > >   vdagentd: move code to do_guest_xorg_resolution()
> > > >   vdagentd: move code to do_agent_file_xfer_status()
> > > >
> > > >  Makefile.am                       |   6 +-
> > > >  configure.ac                      |   2 +-
> > > >  src/udscs.c                       | 560 +++++++--------------------
> > > >  src/udscs.h                       |  58 +--
> > > >  src/vdagent-connection.c          | 301 +++++++++++++++
> > > >  src/vdagent-connection.h          | 103 +++++
> > > >  src/vdagent/vdagent.c             |   3 +-
> > > >  src/vdagent/x11-randr.c           |  43 +--
> > > >  src/vdagentd/console-kit.c        | 613 ++++++++++--------------------
> > > >  src/vdagentd/dummy-session-info.c |   7 +-
> > > >  src/vdagentd/session-info.h       |   6 +-
> > > >  src/vdagentd/systemd-login.c      | 274 +++++--------
> > > >  src/vdagentd/uinput.c             |   9 +-
> > > >  src/vdagentd/vdagentd.c           | 472 +++++++++++------------
> > > >  src/vdagentd/virtio-port.c        | 404 ++++++--------------
> > > >  src/vdagentd/virtio-port.h        |  27 +-
> > > >  16 files changed, 1217 insertions(+), 1671 deletions(-)
> > > >  create mode 100644 src/vdagent-connection.c
> > > >  create mode 100644 src/vdagent-connection.h
> > > >
> > > > --
> > > > 2.17.1
> > > >
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel