<html><body><div style="font-family: times new roman, new york, times, serif; font-size: 12pt; color: #000000"><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div> </div><div class="gmail_quote"><div dir="ltr">On Fri, Oct 13, 2017, 9:29 AM Frediano Ziglio <<a href="mailto:fziglio@redhat.com" target="_blank">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 11, 2017 at 1: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 | 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 <a href="https://cgit.freedesktop.org/~fziglio/vd_agent_linux/commit/?h=janub4&id=cbdff50a3e6e74d7e5e1ff41c806e2ebe25c713b" rel="noreferrer" target="_blank">https://cgit.freedesktop.org/~fziglio/vd_agent_linux/commit/?h=janub4&id=cbdff50a3e6e74d7e5e1ff41c806e2ebe25c713b</a>. 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 </blockquote></div><div> </div><div>I don't like that you encapsulate half of the variables in VDAgent struct and half not.</div><div> I understand that this minimizes the amount of changes, but nevertheless.</div><div>I think we should either make all variables global, or put them all</div><div>(excluding parent_socket, version_mismatch, quit) in the struct.</div><div>I prefer the latter. What do you think?</div><div> </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> </blockquote><div>Fully agree, I should have commented it. </div><div>That's why I didn't posted the patch but a link, not only because I didn't tested it. </div><div>Let me move some field inside the structure. </div><div> </div><div>Frediano </div><div> </div></div></body></html>