[Spice-devel] [PATCH vdagent v2 7/8] vdagent: Use GMainLoop
Frediano Ziglio
fziglio at redhat.com
Wed Oct 4 10:43:34 UTC 2017
>
> 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 | 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.
> 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.
> + 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);
>
> 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);
> - 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
More information about the Spice-devel
mailing list