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