On Mon, Oct 16, 2017 at 4:53 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>
>> From: Jakub Janků <janku.jakub.jj@xxxxxxxxx>
>>
>> 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>
>> Acked-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
>> ---
>> src/udscs.c | 60 ++++++++++++++++++
>> src/vdagent/vdagent.c | 165
>> ++++++++++++++++++++++++++++----------------------
>> 2 files changed, 151 insertions(+), 74 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 69d8786..8d159b0 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 "udscs.h"
>> #include "vdagentd-proto.h"
>> @@ -48,6 +48,11 @@ 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;
>> } VDAgent;
>>
>> static int quit = 0;
>> @@ -177,7 +182,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;
>> @@ -235,25 +240,12 @@ static void daemon_read_complete(struct
>> udscs_connection **connp,
>> }
>> }
>>
>> -static struct udscs_connection *client_setup_sync(void)
>> -{
>> - 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)
>> +static void daemon_disconnect_cb(struct udscs_connection *conn)
>> {
>> - 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
>> @@ -311,10 +303,34 @@ 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;
>> + quit = TRUE;
>> + g_main_loop_quit(agent->loop);
>> + return G_SOURCE_REMOVE;
>> +}
>> +
>> static VDAgent *vdagent_new(void)
>> {
>> VDAgent *agent = g_new0(VDAgent, 1);
>>
>> + agent->loop = g_main_loop_new(NULL, FALSE);
>> +
>> + g_unix_signal_add(SIGINT, vdagent_signal_handler, agent);
>> + g_unix_signal_add(SIGHUP, vdagent_signal_handler, agent);
>> + g_unix_signal_add(SIGTERM, vdagent_signal_handler, agent);
>> +
>> return agent;
>> }
>>
>> @@ -324,14 +340,61 @@ static void vdagent_destroy(VDAgent *agent)
>> vdagent_x11_destroy(agent->x11, agent->conn == NULL);
>> udscs_destroy_connection(&agent->conn);
>>
>> + if (agent->timer_source_id > 0)
>> + g_source_remove(agent->timer_source_id);
>> + if (agent->x11_io_watch_id > 0)
>> + g_source_remove(agent->x11_io_watch_id);
>> + g_clear_pointer(&agent->x11_channel, g_io_channel_unref);
>> + g_clear_pointer(&agent->loop, g_main_loop_unref);
>> g_free(agent);
>> }
>>
>> +static gboolean vdagent_init_async_cb(gpointer user_data)
>> +{
>> + 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 G_SOURCE_CONTINUE;
>> + udscs_set_user_data(agent->conn, agent);
>> +
>> + agent->x11 = vdagent_x11_create(agent->conn, debug, x11_sync);
>> + if (agent->x11 == NULL)
>> + 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");
>> +
>> + if (parent_socket != -1) {
>> + if (write(parent_socket, "OK", 2) != 2)
>> + syslog(LOG_WARNING, "Parent already gone.");
>> + close(parent_socket);
>> + parent_socket = -1;
>> + }
>> +
>> + agent->timer_source_id = 0;
>> + return G_SOURCE_REMOVE;
>> +
>> +err_init:
>> + g_main_loop_quit(agent->loop);
>> + agent->timer_source_id = 0;
>> + quit = TRUE;
>> + return G_SOURCE_REMOVE;
>> +}
>> +
>> int main(int argc, char *argv[])
>> {
>> - fd_set readfds, writefds;
>> - int n, nfds, x11_fd;
>> - struct sigaction act;
>> GOptionContext *context;
>> GError *error = NULL;
>> VDAgent *agent;
>> @@ -357,14 +420,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);
>>
>> @@ -385,53 +440,15 @@ reconnect:
>>
>> agent = vdagent_new();
>>
>> - agent->conn = client_setup_sync();
>> - if (agent->conn == NULL) {
>> - return 1;
>> - }
>> - udscs_set_user_data(agent->conn, agent);
>> -
>> - agent->x11 = vdagent_x11_create(agent->conn, debug, x11_sync);
>> - if (!agent->x11) {
>> - udscs_destroy_connection(&agent->conn);
>> - return 1;
>> - }
>> -
>> - if (!vdagent_init_file_xfer(agent))
>> - syslog(LOG_WARNING, "File transfer is disabled");
>> -
>> - if (parent_socket != -1) {
>> - if (write(parent_socket, "OK", 2) != 2)
>> - syslog(LOG_WARNING, "Parent already gone.");
>> - close(parent_socket);
>> - parent_socket = -1;
>> - }
>> -
>> - 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 (vdagent_init_async_cb(agent) == G_SOURCE_CONTINUE)
>> + agent->timer_source_id = g_timeout_add_seconds(1,
>> vdagent_init_async_cb, agent);
>
> Was thinking about this code.
> Maybe would be better to just register the function with
>
> agent->timer_source_id = g_timeout_add_seconds(0, vdagent_init_async_cb, agent);
>
> and inside if needed register again with 1 second and always returns
> G_SOURCE_REMOVE? That way we always handle signals.
> But possibly vdagent_init_async_cb never blocks so does not matter much.
>
That's actually a good point.
In vdagent_init_async_cb(), vdagent_x11_create() is called,
which can potentially block for up to 1 second, when waiting for
vdagent_x11_get_wm_name().
Current implementation is imho working just fine though.
The only signals we should handle during initialization are SIGINT,
SIGHUP, SIGTERM.
The handler for these signals is registered in vdagent_new(),
that means before the initialization.
If the program is terminated outside main loop, the signal is handled
and marked as pending.
When the initialization finishes (shouldn't take much longer than 1
second at most),
the main loop starts and the signal is delivered to
vdagent_signal_handler(), which exits the loop.
With the current code, the GSource doesn't have to be recreated each time.
I'm not sure whether we should change this, it's up to you.
The signal handlers for SIGINT, SIGHUP, SIGTERM should probably
be removed in vdagent_destroy() too.
Maybe we could remove timer_source_id, x11_io_watch_id and instead
remove all sources using
while (g_source_remove_by_user_data(agent)) {}
>>
>> - if (FD_ISSET(x11_fd, &readfds))
>> - vdagent_x11_do_read(agent->x11);
>> - udscs_client_handle_fds(&agent->conn, &readfds, &writefds);
>> - }
>> + if (!quit)
>> + g_main_loop_run(agent->loop);
>>
>> vdagent_destroy(agent);
>> agent = NULL;
>> +
>> if (!quit && do_daemonize)
>> goto reconnect;
>>
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Jakub
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel