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

[Victor Toso]

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?

> 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

> 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/bdade31a/attachment.sig>