[Spice-devel] [RFC spice-vdagent 00/18] GLib integration
Jakub Janku
jjanku at redhat.com
Mon Sep 3 19:03:38 UTC 2018
On Tue, Aug 28, 2018 at 9:33 AM Victor Toso <victortoso at redhat.com> wrote:
>
> Hi,
>
> First of all, thanks for your work on this :)
>
> I'm still looking into the glib-integration related patches so I
> might still reply once more afterwards. To keep things easy to
> review, I take we could split this series in three different set
> of patches.
>
> - The cleanup code patches, including using glib's memory
> function, should be easy to review an apply
>
> - Glib integration patches, might take some interactions as some
> careful review/testing are needed
>
> - GDBus integration - Shouldn't be hard to review/test but it
> depends on the Glib one.
>
> Would you mind splitting them?
Not a problem.
>
> On Tue, Aug 14, 2018 at 08:53:34PM +0200, Jakub Janků wrote:
> > Hi all,
> >
> > as the subject suggests, this series further deepens the
> > integration of GLib into spice-vdagent(d).
> >
> > Change summary:
> >
> > * udscs.c and virtio-port.c are built upon common base,
> > VDAgentConnection, that handles the lower-level I/O operations
> > * systemd-login.c and console-kit.c use GDBus instead of libdbus
> > * vdagentd.c uses GMainLoop and GLib's commandline option parser
> >
> > Some patches are rather unrelated and try to clean up the code
> > a bit.
> >
> > Where I'd need your help:
> >
> > vdagent_connection_destroy() is in most cases asynchronous (due
> > to the way GCancellable works) and it can therefore take
> > multiple iterations of GMainLoop until the memory associated
> > with VDAgentConnection is freed and the underlying file
> > descriptor gets closed.
> >
> > This is fine as long as the GMainLoop runs. However, the
> > current code assumes, that the call to
> > udscs_destroy_connection() and vdagent_virtio_port_destroy()
> > are synchronous and invokes these functions even after the
> > GMainLoop quits.
> >
> > As a result, the memory allocated by VDAgentConnection might
> > not be explicitely freed and the FD might not be closed, when
> > the spice-vdagent(d) exits.
>
> I'm wondering if making VDAgentConnection into a GObject would
> solve the problem? The vdagent_connection_destroy() would be made
> into g_object_unref(vdagent_connection); When the object is being
> cleanup, that's split into Dispose and Finalize cleaning methods.
>
> So, in the end of GMainLoop, the VDAgentConnection should always
> be correctly finished.
>
> Note that this suggestion is also around how the code has been
> wrote. If you have only one reference to VDAgentConnection, you
> have to be careful when you can dealloc its resources but if you
> _ref() before an operation and _unref() after, you can consider
> that the memory will be there in the callback, simple example:
>
> You have:
>
> g_input_stream_read_all_async(in,
> conn->header_buff,
> conn->header_size,
> G_PRIORITY_DEFAULT,
> conn->cancellable,
> message_read_cb,
> conn);
>
> and in message_read_cb()
>
> VDAgentConnection *conn = user_data;
> ...
> g_input_stream_read_all_finish(in, res, &bytes_read, &err);
> if (err)
> return handle_io_error(conn, err);
>
> where handle_io_error() is doing:
>
> if (g_cancellable_is_cancelled(conn->cancellable)) {
> if (!connection_has_pending(conn))
> connection_finalize(conn);
>
>
> So you have a check to Cancel and after that a
> connection_finalize(), to handle the situation of 'have to
> cleanup my memory right'
>
> Instead, you could have a VDAgentConnection object and the
> initial code would change to:
>
>
> g_input_stream_read_all_async(in,
> ...
> message_read_cb,
> g_object_ref(conn));
>
> and in message_read_cb()
>
> VDAgentConnection *conn = user_data;
> ...
> g_input_stream_read_all_finish(in, res, &bytes_read, &err);
> if (err) {
> if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
> /* print/deal with error message/values */
> }
> /* If needed, this will cleanup everything else, etc. */
> g_object_unref(conn);
> return;
> }
> ...
> g_object_unref(conn);
>
> > Is this an acceptable behavior?
>
> Not one that we shouldn't fix ;)
>
> > Possible solutions:
> > 1) use GMainLoop inside vdagent_connection_destroy() in a
> > similar way as in vdagent_connection_flush() or in the
> > clipboard.c code
> > 2) add a callback for VDAgentConnection's destruction and make
> > the clean-up code at the end of main() run while the GMainLoop
> > is still alive, the callback would then quit it (this would
> > also require adding destruction callbacks for udscs_connection,
> > udscs_server and virtio_port)
> >
> > Is there any better solution?
>
> Overall I think we should GObjectfy if possible as this helps in
> this kind of situation, otherwise we might need to rethink a bit
> the design (not sure about [1] or [2] proposed options)
>
> Cheers,
> Victor
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().
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.
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.
I hope this makes it a bit more clear.
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
More information about the Spice-devel
mailing list