<div class="gmail_quote"><div dir="ltr">On Thu, Oct 5, 2017, 11:20 AM Frediano Ziglio <<a href="mailto:fziglio@redhat.com">fziglio@redhat.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> > On Wed, Oct 4, 2017 at 12:43 PM, Frediano Ziglio <<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>> 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 <<a href="mailto:victortoso@redhat.com" target="_blank">victortoso@redhat.com</a>> > >> --- > >> src/udscs.c | 60 ++++++++++++++++++++++ > >> src/vdagent/vdagent.c | 138 > >> ++++++++++++++++++++++++-------------------------- > >> src/vdagent/vdagent.h | 4 ++ > >> 3 files changed, 131 insertions(+), 71 deletions(-) > >> > >> diff --git a/src/udscs.c b/src/udscs.c > >> index 8b16f89..44842f4 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); > >> > >> + g_clear_pointer(&conn->io_channel, g_io_channel_unref); > >> + 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); > >> + > > > > I would put freeing io_channel as last, but as reference counting > > are used is just style. > > I agree. > > > >> 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 fd2ba51..fe443df 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,13 +290,40 @@ 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; > >> + do_daemonize = FALSE; > > > > why you removed quit and used do_daemonize? > > Is not that readable setting do_daemonize if you want to > > quit. > > > I understand that it's less readable, but do we really need to > keep that extra variable around when the effect of both is the same? > What about creating a simple function like vdagent_exit() > which would quit the mainloop and set do_daemonize to FALSE? > Or renaming do_daemonize to something like keep_running, > or try_reconnect to make it more clear? > #define keep_running do_daemonize ?? or you could release agent->loop and set to NULL so you can check it but this sounds more complicate. > >> + 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); > >> + > >> + g_clear_pointer(&self->x11_channel, g_io_channel_unref); > >> + g_clear_pointer(&self->loop, g_main_loop_unref); > >> + > >> vdagent_finalize_file_xfer(self); > >> vdagent_x11_destroy(self->x11, self->conn == NULL); > >> udscs_destroy_connection(&self->conn); > >> @@ -324,29 +337,50 @@ 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; > >> + > >> + g_io_add_watch(agent->x11_channel, > >> + G_IO_IN, > >> + x11_io_channel_cb, > >> + agent); > >> Just realized a nasty problem with this. This function register a GSource in the default main context so you'll have (references): main_context -> source with x11_io_channel_cb and agent however if you have a reconnection (goto reconnect in main) agent if freed and created again. But the source will still exits with old sources. When loop will be executed potentially these sources will be executed using the old agent pointer which now is invalid. Note that we register the watch for X11 and the timeout for initialization. In case udscs disconnect you'll have the loop recreating the agent. Maybe (not sure) you could use g_main_context_find_source_by_user_data with agent and g_main_context_default to find the sources you should remove (not sure if the user_data in the source is the agent). </blockquote></div><div> </div><div>I think there's an easy fix:</div><div>- both g_io_add_watch() and g_timeout_add_seconds() return a source tag (id)</div><div>- when freeing the agent, the sources can be removed with g_source_remove(id)</div><div> </div><div>We already do this in the udscs.c, have a look. I somehow forgot to add this to the vdagent...</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > >> 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; > >> + } > >> + > >> + return G_SOURCE_REMOVE; > >> + > >> +err_init: > >> + g_main_loop_quit(agent->loop); > >> + do_daemonize = FALSE; > > > > here too. > > > >> + 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 +406,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,44 +426,14 @@ 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; > >> - } > >> > >> - 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; > >> - } > >> + g_timeout_add_seconds(1, vdagent_init_async_cb, agent); > >> > > > > this adds an extra 1 second delay to the start of the agent. > > I understand you did this to handle signals during initialization too. > > An easy fix is to call vdagent_init_async_cb manually, something like > > > > if (vdagent_init_async_cb(agent) != G_SOURCE_CONTINUE) > > g_timeout_add_seconds(1, vdagent_init_async_cb, agent); > > > > if (!quit) > > g_main_loop_run(agent->loop); > > Yes, that would fix it. Maybe we can initialize the agent outside > the mainloop and use sleep() instead (as before). > We probably won't handle signals when the > connection isn't initialized anyway. > > > >> - if (FD_ISSET(x11_fd, &readfds)) > >> - vdagent_x11_do_read(agent->x11); > >> - udscs_client_handle_fds(&agent->conn, &readfds, &writefds); > >> - } > >> + g_main_loop_run(agent->loop); > >> > >> -end_main: > >> g_clear_object(&agent); > >> - if (!quit && do_daemonize) > >> + > >> + if (do_daemonize) > >> goto reconnect; > >> > >> g_free(fx_dir); > >> diff --git a/src/vdagent/vdagent.h b/src/vdagent/vdagent.h > >> index 8376315..2a62f6f 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,6 +40,9 @@ typedef struct VDAgent { > >> struct vdagent_x11 *x11; > >> struct vdagent_file_xfers *xfers; > >> struct udscs_connection *conn; > >> + GIOChannel *x11_channel; > >> + > >> + GMainLoop *loop; > >> > >> int parent_socket; > >> } VDAgent; > > > > Frediano > > Cheers, > Jakub > Frediano </blockquote></div><div> </div><div>Thanks for your review,</div><div> Jakub</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote></div>