[Spice-devel] [PATCH vdagent v3 7/8] vdagent: Use GMainLoop
Frediano Ziglio
fziglio at redhat.com
Wed Oct 11 11:43:16 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 | 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).
> + 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
More information about the Spice-devel
mailing list