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

[Victor Toso]

Hi,

On Tue, Sep 04, 2018 at 02:11:46PM +0200, Jakub Janku wrote:
> > > 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.

How are you checking that any FD is left open? The session agent
would not run without the system one, so it shouldn't be a
problem in case of terminating the process.

The documentation for this GSource points out that a common use
case is to flush things and call g_main_loop_quit().

    https://developer.gnome.org/glib/stable/glib-UNIX-specific-utilities-and-integration.html#g-unix-signal-source-new

Anyway, if needed, you can add a g_idle_add() to g_main_loop_quit
with some comment /* to be sure that GIOStream is closed */
considering that you have called the g_cancellable_cancel(),
it should be plenty.


> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180904/0b1df2c4/attachment-0001.sig>