On Fri, Oct 13, 2017 at 3:49 PM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:On Fri, Oct 13, 2017, 9:29 AM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:>
> On Wed, Oct 11, 2017 at 1:43 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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).FredianoI 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?JakubFully 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)FredianoIt 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
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel