[Spice-devel] [PATCH vdagent v3 7/8] vdagent: Use GMainLoop
Frediano Ziglio
fziglio at redhat.com
Mon Oct 16 14:20:11 UTC 2017
> On Fri, Oct 13, 2017 at 3:49 PM Frediano Ziglio < fziglio at redhat.com > wrote:
> > > > On Fri, Oct 13, 2017, 9:29 AM Frediano Ziglio < fziglio at redhat.com >
> > > > wrote:
> > >
> >
>
> > > > > >
> > > >
> > >
> >
>
> > > > > > On Wed, Oct 11, 2017 at 1:43 PM, Frediano Ziglio <
> > > > > > fziglio at redhat.com
> > > > > > >
> > > > > > wrote:
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> Replace existing main while-loop with GMainLoop.
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> Use GIOChannel with g_io_add_watch() to manage IO
> > > >
> > >
> >
>
> > > > > > >> from x11 connections in the main loop.
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> udscs_connect() now internally creates GSources
> > > >
> > >
> >
>
> > > > > > >> by calling g_io_add_watch().
> > > >
> > >
> >
>
> > > > > > >> This enables GMainLoop integration,
> > > >
> > >
> >
>
> > > > > > >> clients no longer need to call
> > > >
> > >
> >
>
> > > > > > >> udscs_client_fill_fds() and
> > > >
> > >
> >
>
> > > > > > >> udscs_client_handle_fds() in a loop.
> > > >
> > >
> >
>
> > > > > > >> Read callback and udscs_write() functions as before.
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> Signals are also handled in the main loop,
> > > >
> > >
> >
>
> > > > > > >> using g_unix_signal_add().
> > > >
> > >
> >
>
> > > > > > >> SIGQUIT is currently not supported by GLib.
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> This enables further GTK+ integration in the future.
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> Signed-off-by: Victor Toso < victortoso at redhat.com >
> > > >
> > >
> >
>
> > > > > > >> ---
> > > >
> > >
> >
>
> > > > > > >> src/udscs.c | 60 ++++++++++++++++++++
> > > >
> > >
> >
>
> > > > > > >> src/vdagent/vdagent.c | 148
> > > >
> > >
> >
>
> > > > > > >> ++++++++++++++++++++++++++------------------------
> > > >
> > >
> >
>
> > > > > > >> src/vdagent/vdagent.h | 8 +++
> > > >
> > >
> >
>
> > > > > > >> 3 files changed, 145 insertions(+), 71 deletions(-)
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> diff --git a/src/udscs.c b/src/udscs.c
> > > >
> > >
> >
>
> > > > > > >> index 8b16f89..3a1ea44 100644
> > > >
> > >
> >
>
> > > > > > >> --- a/src/udscs.c
> > > >
> > >
> >
>
> > > > > > >> +++ b/src/udscs.c
> > > >
> > >
> >
>
> > > > > > >> @@ -31,6 +31,8 @@
> > > >
> > >
> >
>
> > > > > > >> #include <errno.h>
> > > >
> > >
> >
>
> > > > > > >> #include <sys/socket.h>
> > > >
> > >
> >
>
> > > > > > >> #include <sys/un.h>
> > > >
> > >
> >
>
> > > > > > >> +#include <glib.h>
> > > >
> > >
> >
>
> > > > > > >> +#include <glib-unix.h>
> > > >
> > >
> >
>
> > > > > > >> #include "udscs.h"
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> struct udscs_buf {
> > > >
> > >
> >
>
> > > > > > >> @@ -66,8 +68,16 @@ struct udscs_connection {
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> struct udscs_connection *next;
> > > >
> > >
> >
>
> > > > > > >> struct udscs_connection *prev;
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> + GIOChannel *io_channel;
> > > >
> > >
> >
>
> > > > > > >> + guint write_watch_id;
> > > >
> > >
> >
>
> > > > > > >> + guint read_watch_id;
> > > >
> > >
> >
>
> > > > > > >> };
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> +static gboolean udscs_io_channel_cb(GIOChannel *source,
> > > >
> > >
> >
>
> > > > > > >> + GIOCondition condition,
> > > >
> > >
> >
>
> > > > > > >> + gpointer data);
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> struct udscs_connection *udscs_connect(const char *socketname,
> > > >
> > >
> >
>
> > > > > > >> udscs_read_callback read_callback,
> > > >
> > >
> >
>
> > > > > > >> udscs_disconnect_callback disconnect_callback,
> > > >
> > >
> >
>
> > > > > > >> @@ -103,6 +113,17 @@ struct udscs_connection
> > > > > > >> *udscs_connect(const
> > > > > > >> char
> > > >
> > >
> >
>
> > > > > > >> *socketname,
> > > >
> > >
> >
>
> > > > > > >> return NULL;
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> + conn->io_channel = g_io_channel_unix_new(conn->fd);
> > > >
> > >
> >
>
> > > > > > >> + if (!conn->io_channel) {
> > > >
> > >
> >
>
> > > > > > >> + udscs_destroy_connection(&conn);
> > > >
> > >
> >
>
> > > > > > >> + return NULL;
> > > >
> > >
> >
>
> > > > > > >> + }
> > > >
> > >
> >
>
> > > > > > >> + conn->read_watch_id =
> > > >
> > >
> >
>
> > > > > > >> + g_io_add_watch(conn->io_channel,
> > > >
> > >
> >
>
> > > > > > >> + G_IO_IN | G_IO_ERR | G_IO_NVAL,
> > > >
> > >
> >
>
> > > > > > >> + udscs_io_channel_cb,
> > > >
> > >
> >
>
> > > > > > >> + conn);
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> conn->read_callback = read_callback;
> > > >
> > >
> >
>
> > > > > > >> conn->disconnect_callback = disconnect_callback;
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> @@ -141,6 +162,12 @@ void udscs_destroy_connection(struct
> > > > > > >> udscs_connection
> > > >
> > >
> >
>
> > > > > > >> **connp)
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> close(conn->fd);
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> + if (conn->write_watch_id != 0)
> > > >
> > >
> >
>
> > > > > > >> + g_source_remove(conn->write_watch_id);
> > > >
> > >
> >
>
> > > > > > >> + if (conn->read_watch_id != 0)
> > > >
> > >
> >
>
> > > > > > >> + g_source_remove(conn->read_watch_id);
> > > >
> > >
> >
>
> > > > > > >> + g_clear_pointer(&conn->io_channel, g_io_channel_unref);
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> if (conn->debug)
> > > >
> > >
> >
>
> > > > > > >> syslog(LOG_DEBUG, "%p disconnected", conn);
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> @@ -198,6 +225,13 @@ int udscs_write(struct udscs_connection
> > > > > > >> *conn,
> > > >
> > >
> >
>
> > > > > > >> uint32_t
> > > >
> > >
> >
>
> > > > > > >> type, uint32_t arg1,
> > > >
> > >
> >
>
> > > > > > >> conn, type, arg1, arg2, size);
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> + if (conn->io_channel && conn->write_watch_id == 0)
> > > >
> > >
> >
>
> > > > > > >> + conn->write_watch_id =
> > > >
> > >
> >
>
> > > > > > >> + g_io_add_watch(conn->io_channel,
> > > >
> > >
> >
>
> > > > > > >> + G_IO_OUT | G_IO_ERR | G_IO_NVAL,
> > > >
> > >
> >
>
> > > > > > >> + udscs_io_channel_cb,
> > > >
> > >
> >
>
> > > > > > >> + conn);
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> if (!conn->write_buf) {
> > > >
> > >
> >
>
> > > > > > >> conn->write_buf = new_wbuf;
> > > >
> > >
> >
>
> > > > > > >> return 0;
> > > >
> > >
> >
>
> > > > > > >> @@ -353,6 +387,32 @@ int udscs_client_fill_fds(struct
> > > > > > >> udscs_connection
> > > >
> > >
> >
>
> > > > > > >> *conn,
> > > >
> > >
> >
>
> > > > > > >> fd_set *readfds,
> > > >
> > >
> >
>
> > > > > > >> return conn->fd + 1;
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> +static gboolean udscs_io_channel_cb(GIOChannel *source,
> > > >
> > >
> >
>
> > > > > > >> + GIOCondition condition,
> > > >
> > >
> >
>
> > > > > > >> + gpointer data)
> > > >
> > >
> >
>
> > > > > > >> +{
> > > >
> > >
> >
>
> > > > > > >> + struct udscs_connection *conn = data;
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> + if (condition & G_IO_IN) {
> > > >
> > >
> >
>
> > > > > > >> + udscs_do_read(&conn);
> > > >
> > >
> >
>
> > > > > > >> + if (conn == NULL)
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_REMOVE;
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_CONTINUE;
> > > >
> > >
> >
>
> > > > > > >> + }
> > > >
> > >
> >
>
> > > > > > >> + if (condition & G_IO_OUT) {
> > > >
> > >
> >
>
> > > > > > >> + udscs_do_write(&conn);
> > > >
> > >
> >
>
> > > > > > >> + if (conn == NULL)
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_REMOVE;
> > > >
> > >
> >
>
> > > > > > >> + if (conn->write_buf)
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_CONTINUE;
> > > >
> > >
> >
>
> > > > > > >> + conn->write_watch_id = 0;
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_REMOVE;
> > > >
> > >
> >
>
> > > > > > >> + }
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> + udscs_destroy_connection(&conn);
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_REMOVE;
> > > >
> > >
> >
>
> > > > > > >> +}
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> #ifndef UDSCS_NO_SERVER
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> > > >
> > >
> >
>
> > > > > > >> index 462b5fe..3474f17 100644
> > > >
> > >
> >
>
> > > > > > >> --- a/src/vdagent/vdagent.c
> > > >
> > >
> >
>
> > > > > > >> +++ b/src/vdagent/vdagent.c
> > > >
> > >
> >
>
> > > > > > >> @@ -34,8 +34,8 @@
> > > >
> > >
> >
>
> > > > > > >> #include <sys/select.h>
> > > >
> > >
> >
>
> > > > > > >> #include <sys/stat.h>
> > > >
> > >
> >
>
> > > > > > >> #include <spice/vd_agent.h>
> > > >
> > >
> >
>
> > > > > > >> -#include <glib.h>
> > > >
> > >
> >
>
> > > > > > >> #include <poll.h>
> > > >
> > >
> >
>
> > > > > > >> +#include <glib-unix.h>
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> #include "vdagentd-proto.h"
> > > >
> > >
> >
>
> > > > > > >> #include "vdagentd-proto-strings.h"
> > > >
> > >
> >
>
> > > > > > >> @@ -44,7 +44,6 @@
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> G_DEFINE_TYPE (VDAgent, vdagent, G_TYPE_OBJECT);
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> -static int quit = 0;
> > > >
> > >
> >
>
> > > > > > >> static int version_mismatch = 0;
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> /* Command line options */
> > > >
> > >
> >
>
> > > > > > >> @@ -170,7 +169,7 @@ static void daemon_read_complete(struct
> > > >
> > >
> >
>
> > > > > > >> udscs_connection
> > > >
> > >
> >
>
> > > > > > >> **connp,
> > > >
> > >
> >
>
> > > > > > >> if (strcmp((char *)data, VERSION) != 0) {
> > > >
> > >
> >
>
> > > > > > >> syslog(LOG_INFO, "vdagentd version mismatch: got %s expected
> > > >
> > >
> >
>
> > > > > > >> %s",
> > > >
> > >
> >
>
> > > > > > >> data, VERSION);
> > > >
> > >
> >
>
> > > > > > >> - udscs_destroy_connection(connp);
> > > >
> > >
> >
>
> > > > > > >> + g_main_loop_quit(agent->loop);
> > > >
> > >
> >
>
> > > > > > >> version_mismatch = 1;
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >> break;
> > > >
> > >
> >
>
> > > > > > >> @@ -228,25 +227,12 @@ static void daemon_read_complete(struct
> > > >
> > >
> >
>
> > > > > > >> udscs_connection **connp,
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> -static struct udscs_connection *client_setup_sync(VDAgent
> > > > > > >> *agent)
> > > >
> > >
> >
>
> > > > > > >> +static void daemon_disconnect_cb(struct udscs_connection *conn)
> > > >
> > >
> >
>
> > > > > > >> {
> > > >
> > >
> >
>
> > > > > > >> - struct udscs_connection *conn = NULL;
> > > >
> > >
> >
>
> > > > > > >> -
> > > >
> > >
> >
>
> > > > > > >> - while (!quit) {
> > > >
> > >
> >
>
> > > > > > >> - conn = udscs_connect(vdagentd_socket, daemon_read_complete,
> > > > > > >> NULL,
> > > >
> > >
> >
>
> > > > > > >> - vdagentd_messages, VDAGENTD_NO_MESSAGES,
> > > >
> > >
> >
>
> > > > > > >> - debug);
> > > >
> > >
> >
>
> > > > > > >> - if (conn || !do_daemonize || quit) {
> > > >
> > >
> >
>
> > > > > > >> - break;
> > > >
> > >
> >
>
> > > > > > >> - }
> > > >
> > >
> >
>
> > > > > > >> - sleep(1);
> > > >
> > >
> >
>
> > > > > > >> - }
> > > >
> > >
> >
>
> > > > > > >> - return conn;
> > > >
> > >
> >
>
> > > > > > >> -}
> > > >
> > >
> >
>
> > > > > > >> -
> > > >
> > >
> >
>
> > > > > > >> -static void quit_handler(int sig)
> > > >
> > >
> >
>
> > > > > > >> -{
> > > >
> > >
> >
>
> > > > > > >> - quit = 1;
> > > >
> > >
> >
>
> > > > > > >> + VDAgent *agent = udscs_get_user_data(conn);
> > > >
> > >
> >
>
> > > > > > >> + agent->conn = NULL;
> > > >
> > >
> >
>
> > > > > > >> + if (agent->loop)
> > > >
> > >
> >
>
> > > > > > >> + g_main_loop_quit(agent->loop);
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> /* When we daemonize, it is useful to have the main process
> > > >
> > >
> >
>
> > > > > > >> @@ -304,17 +290,48 @@ static int file_test(const char *path)
> > > >
> > >
> >
>
> > > > > > >> return stat(path, &buffer);
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> +static gboolean x11_io_channel_cb(GIOChannel *source,
> > > >
> > >
> >
>
> > > > > > >> + GIOCondition condition,
> > > >
> > >
> >
>
> > > > > > >> + gpointer data)
> > > >
> > >
> >
>
> > > > > > >> +{
> > > >
> > >
> >
>
> > > > > > >> + VDAgent *agent = data;
> > > >
> > >
> >
>
> > > > > > >> + vdagent_x11_do_read(agent->x11);
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_CONTINUE;
> > > >
> > >
> >
>
> > > > > > >> +}
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> +gboolean vdagent_signal_handler(gpointer user_data)
> > > >
> > >
> >
>
> > > > > > >> +{
> > > >
> > >
> >
>
> > > > > > >> + VDAgent *agent = user_data;
> > > >
> > >
> >
>
> > > > > > >> + agent->quit = TRUE;
> > > >
> > >
> >
>
> > > > > > >> + g_main_loop_quit(agent->loop);
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_REMOVE;
> > > >
> > >
> >
>
> > > > > > >> +}
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> static void vdagent_init(VDAgent *self)
> > > >
> > >
> >
>
> > > > > > >> {
> > > >
> > >
> >
>
> > > > > > >> + self->loop = g_main_loop_new(NULL, FALSE);
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> + g_unix_signal_add(SIGINT, vdagent_signal_handler, self);
> > > >
> > >
> >
>
> > > > > > >> + g_unix_signal_add(SIGHUP, vdagent_signal_handler, self);
> > > >
> > >
> >
>
> > > > > > >> + g_unix_signal_add(SIGTERM, vdagent_signal_handler, self);
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> static void vdagent_finalize(GObject *gobject)
> > > >
> > >
> >
>
> > > > > > >> {
> > > >
> > >
> >
>
> > > > > > >> VDAgent *self = VDAGENT(gobject);
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> vdagent_finalize_file_xfer(self);
> > > >
> > >
> >
>
> > > > > > >> vdagent_x11_destroy(self->x11, self->conn == NULL);
> > > >
> > >
> >
>
> > > > > > >> udscs_destroy_connection(&self->conn);
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> + if (self->timer_source_id > 0)
> > > >
> > >
> >
>
> > > > > > >> + g_source_remove(self->timer_source_id);
> > > >
> > >
> >
>
> > > > > > >> + if (self->x11_io_watch_id > 0)
> > > >
> > >
> >
>
> > > > > > >> + g_source_remove(self->x11_io_watch_id);
> > > >
> > >
> >
>
> > > > > > >> + g_clear_pointer(&self->x11_channel, g_io_channel_unref);
> > > >
> > >
> >
>
> > > > > > >> + g_clear_pointer(&self->loop, g_main_loop_unref);
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> G_OBJECT_CLASS(vdagent_parent_class)->finalize(gobject);
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> @@ -324,29 +341,53 @@ static void
> > > > > > >> vdagent_class_init(VDAgentClass
> > > > > > >> *klass)
> > > >
> > >
> >
>
> > > > > > >> gobject_class->finalize = vdagent_finalize;
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> -static gboolean vdagent_init_sync(VDAgent *agent)
> > > >
> > >
> >
>
> > > > > > >> +static gboolean vdagent_init_async_cb(gpointer user_data)
> > > >
> > >
> >
>
> > > > > > >> {
> > > >
> > >
> >
>
> > > > > > >> - agent->conn = client_setup_sync(agent);
> > > >
> > >
> >
>
> > > > > > >> + VDAgent *agent = user_data;
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> + agent->conn = udscs_connect(vdagentd_socket,
> > > >
> > >
> >
>
> > > > > > >> + daemon_read_complete,
> > > >
> > >
> >
>
> > > > > > >> daemon_disconnect_cb,
> > > >
> > >
> >
>
> > > > > > >> + vdagentd_messages, VDAGENTD_NO_MESSAGES,
> > > >
> > >
> >
>
> > > > > > >> debug);
> > > >
> > >
> >
>
> > > > > > >> if (agent->conn == NULL)
> > > >
> > >
> >
>
> > > > > > >> - return FALSE;
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_CONTINUE;
> > > >
> > >
> >
>
> > > > > > >> udscs_set_user_data(agent->conn, agent);
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> agent->x11 = vdagent_x11_create(agent->conn, debug, x11_sync);
> > > >
> > >
> >
>
> > > > > > >> if (agent->x11 == NULL)
> > > >
> > >
> >
>
> > > > > > >> - return FALSE;
> > > >
> > >
> >
>
> > > > > > >> + goto err_init;
> > > >
> > >
> >
>
> > > > > > >> + agent->x11_channel =
> > > >
> > >
> >
>
> > > > > > >> g_io_channel_unix_new(vdagent_x11_get_fd(agent->x11));
> > > >
> > >
> >
>
> > > > > > >> + if (agent->x11_channel == NULL)
> > > >
> > >
> >
>
> > > > > > >> + goto err_init;
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> + agent->x11_io_watch_id =
> > > >
> > >
> >
>
> > > > > > >> + g_io_add_watch(agent->x11_channel,
> > > >
> > >
> >
>
> > > > > > >> + G_IO_IN,
> > > >
> > >
> >
>
> > > > > > >> + x11_io_channel_cb,
> > > >
> > >
> >
>
> > > > > > >> + agent);
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> if (!vdagent_init_file_xfer(agent))
> > > >
> > >
> >
>
> > > > > > >> syslog(LOG_WARNING, "File transfer is disabled");
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> - return TRUE;
> > > >
> > >
> >
>
> > > > > > >> + if (agent->parent_socket) {
> > > >
> > >
> >
>
> > > > > > >> + if (write(agent->parent_socket, "OK", 2) != 2)
> > > >
> > >
> >
>
> > > > > > >> + syslog(LOG_WARNING, "Parent already gone.");
> > > >
> > >
> >
>
> > > > > > >> + close(agent->parent_socket);
> > > >
> > >
> >
>
> > > > > > >> + agent->parent_socket = 0;
> > > >
> > >
> >
>
> > > > > > >
> > > >
> > >
> >
>
> > > > > > > This is introducing a bug. agent->parent_socket is set to 0
> > > >
> > >
> >
>
> > > > > > > but on next program iteration (in main) agent->parent_socket
> > > >
> > >
> >
>
> > > > > > > will be set to old parent_socket which in the meantime has been
> > > >
> > >
> >
>
> > > > > > > closed. This potentially will trigger some operation on a
> > > > > > > different
> > > >
> > >
> >
>
> > > > > > > file descriptor.
> > > >
> > >
> >
>
> > > > > > >
> > > >
> > >
> >
>
> > > > > > > Maybe better to use a global "parent_socket" variable.
> > > >
> > >
> >
>
> > > > > > > Would also be better to use -1 as invalid value instead of 0,
> > > >
> > >
> >
>
> > > > > > > but this is not a regression.
> > > >
> > >
> >
>
> > > > > > >
> > > >
> > >
> >
>
> > > > > > >> + }
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> + agent->timer_source_id = 0;
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_REMOVE;
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> +err_init:
> > > >
> > >
> >
>
> > > > > > >> + g_main_loop_quit(agent->loop);
> > > >
> > >
> >
>
> > > > > > >> + agent->timer_source_id = 0;
> > > >
> > >
> >
>
> > > > > > >> + agent->quit = TRUE;
> > > >
> > >
> >
>
> > > > > > >> + return G_SOURCE_REMOVE;
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> int main(int argc, char *argv[])
> > > >
> > >
> >
>
> > > > > > >> {
> > > >
> > >
> >
>
> > > > > > >> - fd_set readfds, writefds;
> > > >
> > >
> >
>
> > > > > > >> - int n, nfds, x11_fd;
> > > >
> > >
> >
>
> > > > > > >> int parent_socket = 0;
> > > >
> > >
> >
>
> > > > > > >> - struct sigaction act;
> > > >
> > >
> >
>
> > > > > > >> GOptionContext *context;
> > > >
> > >
> >
>
> > > > > > >> GError *error = NULL;
> > > >
> > >
> >
>
> > > > > > >> VDAgent *agent;
> > > >
> > >
> >
>
> > > > > > >> @@ -372,14 +413,6 @@ int main(int argc, char *argv[])
> > > >
> > >
> >
>
> > > > > > >> if (vdagentd_socket == NULL)
> > > >
> > >
> >
>
> > > > > > >> vdagentd_socket = g_strdup(VDAGENTD_SOCKET);
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> - memset(&act, 0, sizeof(act));
> > > >
> > >
> >
>
> > > > > > >> - act.sa_flags = SA_RESTART;
> > > >
> > >
> >
>
> > > > > > >> - act.sa_handler = quit_handler;
> > > >
> > >
> >
>
> > > > > > >> - sigaction(SIGINT, &act, NULL);
> > > >
> > >
> >
>
> > > > > > >> - sigaction(SIGHUP, &act, NULL);
> > > >
> > >
> >
>
> > > > > > >> - sigaction(SIGTERM, &act, NULL);
> > > >
> > >
> >
>
> > > > > > >> - sigaction(SIGQUIT, &act, NULL);
> > > >
> > >
> >
>
> > > > > > >> -
> > > >
> > >
> >
>
> > > > > > >> openlog("spice-vdagent", do_daemonize ? LOG_PID : (LOG_PID |
> > > >
> > >
> >
>
> > > > > > >> LOG_PERROR),
> > > >
> > >
> >
>
> > > > > > >> LOG_USER);
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> @@ -400,45 +433,18 @@ reconnect:
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> agent = g_object_new(TYPE_VDAGENT, NULL);
> > > >
> > >
> >
>
> > > > > > >> agent->parent_socket = parent_socket;
> > > >
> > >
> >
>
> > > > > > >> - if (!vdagent_init_sync(agent)) {
> > > >
> > >
> >
>
> > > > > > >> - quit = 1;
> > > >
> > >
> >
>
> > > > > > >> - goto end_main;
> > > >
> > >
> >
>
> > > > > > >> - }
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> - if (parent_socket) {
> > > >
> > >
> >
>
> > > > > > >> - if (write(parent_socket, "OK", 2) != 2)
> > > >
> > >
> >
>
> > > > > > >> - syslog(LOG_WARNING, "Parent already gone.");
> > > >
> > >
> >
>
> > > > > > >> - close(parent_socket);
> > > >
> > >
> >
>
> > > > > > >> - parent_socket = 0;
> > > >
> > >
> >
>
> > > > > > >> - }
> > > >
> > >
> >
>
> > > > > > >> + if (vdagent_init_async_cb(agent) == G_SOURCE_CONTINUE)
> > > >
> > >
> >
>
> > > > > > >> + agent->timer_source_id = g_timeout_add_seconds(1,
> > > >
> > >
> >
>
> > > > > > >> vdagent_init_async_cb, agent);
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> - while (agent->conn && !quit) {
> > > >
> > >
> >
>
> > > > > > >> - FD_ZERO(&readfds);
> > > >
> > >
> >
>
> > > > > > >> - FD_ZERO(&writefds);
> > > >
> > >
> >
>
> > > > > > >> -
> > > >
> > >
> >
>
> > > > > > >> - nfds = udscs_client_fill_fds(agent->conn, &readfds,
> > > > > > >> &writefds);
> > > >
> > >
> >
>
> > > > > > >> - x11_fd = vdagent_x11_get_fd(agent->x11);
> > > >
> > >
> >
>
> > > > > > >> - FD_SET(x11_fd, &readfds);
> > > >
> > >
> >
>
> > > > > > >> - if (x11_fd >= nfds)
> > > >
> > >
> >
>
> > > > > > >> - nfds = x11_fd + 1;
> > > >
> > >
> >
>
> > > > > > >> -
> > > >
> > >
> >
>
> > > > > > >> - n = select(nfds, &readfds, &writefds, NULL, NULL);
> > > >
> > >
> >
>
> > > > > > >> - if (n == -1) {
> > > >
> > >
> >
>
> > > > > > >> - if (errno == EINTR)
> > > >
> > >
> >
>
> > > > > > >> - continue;
> > > >
> > >
> >
>
> > > > > > >> - syslog(LOG_ERR, "Fatal error select: %s", strerror(errno));
> > > >
> > >
> >
>
> > > > > > >> - break;
> > > >
> > >
> >
>
> > > > > > >> - }
> > > >
> > >
> >
>
> > > > > > >> + if (!agent->quit)
> > > >
> > >
> >
>
> > > > > > >> + g_main_loop_run(agent->loop);
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> - if (FD_ISSET(x11_fd, &readfds))
> > > >
> > >
> >
>
> > > > > > >> - vdagent_x11_do_read(agent->x11);
> > > >
> > >
> >
>
> > > > > > >> - udscs_client_handle_fds(&agent->conn, &readfds, &writefds);
> > > >
> > >
> >
>
> > > > > > >> + if (!agent->quit && do_daemonize) {
> > > >
> > >
> >
>
> > > > > > >
> > > >
> > >
> >
>
> > > > > > > Maybe would be better to use the old global "quit" variable
> > > > > > > instead?
> > > >
> > >
> >
>
> > > > > > >
> > > >
> > >
> >
>
> > > > > > > I think all these regression came from the fact the VDAgent is
> > > > > > > badly
> > > >
> > >
> >
>
> > > > > > > incapsulating
> > > >
> > >
> >
>
> > > > > > > the program. If is representing the entire program should not be
> > > > > > > destroyed
> > > >
> > >
> >
>
> > > > > > > and created again but destroyed only when program exit.
> > > >
> > >
> >
>
> > > > > > >
> > > >
> > >
> >
>
> > > > > > > Is basically representing some partial state of the program but
> > > > > > > also
> > > >
> > >
> >
>
> > > > > > > implementing the program itself. This is the reason you need to
> > > > > > > save
> > > > > > > some
> > > >
> > >
> >
>
> > > > > > > state and restore it (quit, parent_socket, version_mismatch).
> > > >
> > >
> >
>
> > > > > >
> > > >
> > >
> >
>
> > > > > > I would make the variables quit, parent_socket, version_mismatch
> > > > > > global,
> > > >
> > >
> >
>
> > > > > > as you suggested.
> > > >
> > >
> >
>
> > > > > > This way the VDAgent object would represent one "program
> > > > > > iteration",
> > > >
> > >
> >
>
> > > > > > which makes perfect sense to me.
> > > >
> > >
> >
>
> > > > > > Would you be OK with that, or do you think bigger refactor should
> > > > > > be
> > > >
> > >
> >
>
> > > > > > done when you say
> > > >
> > >
> >
>
> > > > > > that "VDAgent is badly incapsulating the program"?
> > > >
> > >
> >
>
> > > > > >
> > > >
> > >
> >
>
> > > > > > >
> > > >
> > >
> >
>
> > > > > > >> + g_clear_object(&agent);
> > > >
> > >
> >
>
> > > > > > >> + goto reconnect;
> > > >
> > >
> >
>
> > > > > > >> }
> > > >
> > >
> >
>
> > > > > > >> -
> > > >
> > >
> >
>
> > > > > > >> -end_main:
> > > >
> > >
> >
>
> > > > > > >> g_clear_object(&agent);
> > > >
> > >
> >
>
> > > > > > >> - if (!quit && do_daemonize)
> > > >
> > >
> >
>
> > > > > > >> - goto reconnect;
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> g_free(fx_dir);
> > > >
> > >
> >
>
> > > > > > >> g_free(portdev);
> > > >
> > >
> >
>
> > > > > > >> diff --git a/src/vdagent/vdagent.h b/src/vdagent/vdagent.h
> > > >
> > >
> >
>
> > > > > > >> index 8376315..cd1d788 100644
> > > >
> > >
> >
>
> > > > > > >> --- a/src/vdagent/vdagent.h
> > > >
> > >
> >
>
> > > > > > >> +++ b/src/vdagent/vdagent.h
> > > >
> > >
> >
>
> > > > > > >> @@ -20,6 +20,7 @@
> > > >
> > >
> >
>
> > > > > > >> #define __VDAGENT_H_
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> #include <glib-object.h>
> > > >
> > >
> >
>
> > > > > > >> +#include <glib.h>
> > > >
> > >
> >
>
> > > > > > >> #include "udscs.h"
> > > >
> > >
> >
>
> > > > > > >> #include "x11.h"
> > > >
> > >
> >
>
> > > > > > >> #include "file-xfers.h"
> > > >
> > >
> >
>
> > > > > > >> @@ -39,8 +40,15 @@ typedef struct VDAgent {
> > > >
> > >
> >
>
> > > > > > >> struct vdagent_x11 *x11;
> > > >
> > >
> >
>
> > > > > > >> struct vdagent_file_xfers *xfers;
> > > >
> > >
> >
>
> > > > > > >> struct udscs_connection *conn;
> > > >
> > >
> >
>
> > > > > > >> + GIOChannel *x11_channel;
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> + GMainLoop *loop;
> > > >
> > >
> >
>
> > > > > > >> + guint timer_source_id;
> > > >
> > >
> >
>
> > > > > > >> + guint x11_io_watch_id;
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> int parent_socket;
> > > >
> > >
> >
>
> > > > > > >> +
> > > >
> > >
> >
>
> > > > > > >> + gboolean quit;
> > > >
> > >
> >
>
> > > > > > >> } VDAgent;
> > > >
> > >
> >
>
> > > > > > >>
> > > >
> > >
> >
>
> > > > > > >> typedef struct VDAgentClass {
> > > >
> > >
> >
>
> > > > > > >
> > > >
> > >
> >
>
> > > > > > > Frediano
> > > >
> > >
> >
>
> > > > > >
> > > >
> > >
> >
>
> > > > > > Thanks,
> > > >
> > >
> >
>
> > > > > > Jakub
> > > >
> > >
> >
>
> > > > > >
> > > >
> > >
> >
>
> > > > > Was looking at something more radical like removing entirely 6/8,
> > > >
> > >
> >
>
> > > > > see
> > > > > https://cgit.freedesktop.org/~fziglio/vd_agent_linux/commit/?h=janub4&id=cbdff50a3e6e74d7e5e1ff41c806e2ebe25c713b
> > > > > .
> > > >
> > >
> >
>
> > > > > I still didn't test it. Looking back at at 6/8 after making udcsc
> > > > > code
> > > > > independent
> > > >
> > >
> >
>
> > > > > again GObject does not make any improvement adding 80 lines for no
> > > > > features
> > > > > and
> > > >
> > >
> >
>
> > > > > an extra dependencies (GObject library).
> > > >
> > >
> >
>
> > > > > Frediano
> > > >
> > >
> >
>
> > > > I don't like that you encapsulate half of the variables in VDAgent
> > > > struct
> > > > and
> > > > half not.
> > >
> >
>
> > > > I understand that this minimizes the amount of changes, but
> > > > nevertheless.
> > >
> >
>
> > > > I think we should either make all variables global, or put them all
> > >
> >
>
> > > > (excluding parent_socket, version_mismatch, quit) in the struct.
> > >
> >
>
> > > > I prefer the latter. What do you think?
> > >
> >
>
> > > > Jakub
> > >
> >
>
> > > Fully agree, I should have commented it.
> >
>
> > > That's why I didn't posted the patch but a link, not only because I
> > > didn't
> > > tested it.
> >
>
> > > Let me move some field inside the structure.
> >
>
> > Ok, got some code, still to test, see
> > https://cgit.freedesktop.org/~fziglio/vd_agent_linux/log/?h=janub4
>
> > (changed patches 6 and 7)
>
> > Frediano
>
> It looks good to me.
> I just noticed one mistake:
> - in vdagent_init_file_xfer(), we don't need to set agent->xfers = NULL,
> since we already syslog and return when it isn't NULL.
> This goes back to the commit "vdagent: move file xfer initialization",
> but I removed it in "vdagent: Introduce VDAgent object".
> My mistake, sorry.
> Jakub
Removed the offending line.
Tested and working fine.
Do you want me to send a new version based on my replacements or do you want to pick up
these patches?
Frediano
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171016/215ff64e/attachment-0001.html>
More information about the Spice-devel
mailing list