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?
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
> Cheers,
> 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