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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel