[Spice-devel] [PATCH vdagent v2 7/8] vdagent: Use GMainLoop
Jakub Janků
janku.jakub.jj at gmail.com
Wed Oct 4 22:37:19 UTC 2017
On Wed, Oct 4, 2017 at 12:43 PM, Frediano Ziglio <fziglio at redhat.com> 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 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.
I agree.
>
>> 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.
>
I understand that it's less readable, but do we really need to
keep that extra variable around when the effect of both is the same?
What about creating a simple function like vdagent_exit()
which would quit the mainloop and set do_daemonize to FALSE?
Or renaming do_daemonize to something like keep_running,
or try_reconnect to make it more clear?
>> + 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);
Yes, that would fix it. Maybe we can initialize the agent outside
the mainloop and use sleep() instead (as before).
We probably won't handle signals when the
connection isn't initialized anyway.
>
>> - 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
Cheers,
Jakub
More information about the Spice-devel
mailing list