[Spice-devel] [PATCH v5 7/8] vdagent: Use GMainLoop
Frediano Ziglio
fziglio at redhat.com
Wed Oct 18 08:13:17 UTC 2017
> On Wed, Oct 18, 2017, 9:17 AM Frediano Ziglio < fziglio at redhat.com > wrote:
> > >
>
> > > On Tue, Oct 17, 2017 at 10:09 AM, Frediano Ziglio < fziglio at redhat.com >
> > > wrote:
>
> > > > From: Jakub Janků < janku.jakub.jj at gmail.com >
>
> > > >
>
> > > > 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 | 159
>
> > > > +++++++++++++++++++++++++++-----------------------
>
> > > > 2 files changed, 145 insertions(+), 74 deletions(-)
>
> > > >
>
> > > > Changes since v4:
>
> > > > - remove source ids and use g_source_remove_by_user_data to remove
>
> > > > all sources, even signal ones;
>
> > > > - register first connection event with g_timeout_add instead
>
> > > > of calling the callback directly.
>
> > > >
>
> > > > 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..ac90634 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,9 @@ typedef struct VDAgent {
>
> > > > struct vdagent_x11 *x11;
>
> > > > struct vdagent_file_xfers *xfers;
>
> > > > struct udscs_connection *conn;
>
> > > > + GIOChannel *x11_channel;
>
> > > > +
>
> > > > + GMainLoop *loop;
>
> > > > } VDAgent;
>
> > > >
>
> > > > static int quit = 0;
>
> > > > @@ -177,7 +180,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 +238,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 +301,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 +338,59 @@ static void vdagent_destroy(VDAgent *agent)
>
> > > > vdagent_x11_destroy(agent->x11, agent->conn == NULL);
>
> > > > udscs_destroy_connection(&agent->conn);
>
> > > >
>
> > > > + while (g_source_remove_by_user_data(agent))
>
> > > > + continue;
>
> > > > +
>
> > > > + 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) {
>
> > > > + g_timeout_add_seconds(1, vdagent_init_async_cb, agent);
>
> > > > + return G_SOURCE_REMOVE;
>
> > > > + }
>
> > > > + 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;
>
> > > > +
>
> > > > + 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;
>
> > > > + }
>
> > > > +
>
> > > > + return G_SOURCE_REMOVE;
>
> > > > +
>
> > > > +err_init:
>
> > > > + g_main_loop_quit(agent->loop);
>
> > > > + 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 +416,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 +436,13 @@ reconnect:
>
> > > >
>
> > > > agent = vdagent_new();
>
> > > >
>
> > > > - agent->conn = client_setup_sync();
>
> > > > - if (agent->conn == NULL) {
>
> > > > - return 1;
>
> > > > - }
>
> > > > - udscs_set_user_data(agent->conn, agent);
>
> > > > + g_timeout_add(0, vdagent_init_async_cb, 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 (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);
>
> > > >
>
> > > > vdagent_destroy(agent);
>
> > > > agent = NULL;
>
> > > > +
>
> > > > if (!quit && do_daemonize)
>
> > > > goto reconnect;
>
> > > >
>
> > > > --
>
> > > > 2.13.6
>
> > > >
>
> > >
>
> > > Just one more note on this:
>
> > > According to GTK docs for g_timeout_add_seconds():
>
> > > "Note that the first call of the timer may not be precise for timeouts
>
> > > of one second.
>
> > > If you need finer precision and have such a timeout,
>
> > > you may want to use g_timeout_add() instead."
>
> > > (this is basically the same what Christophe pointed out)
>
> > >
>
> > Yes, first iteration uses (last patch version) g_timeout_add.
>
> > > Since we remove the source in vdagent_init_async_cb() each time,
>
> > > the interval between each call might not be regular.
>
> > >
>
> > From g_timeout_add_seconds_full documentation:
>
> > "Note that timeout functions may be delayed, due to the processing of other
>
> > event sources. Thus they should not be relied on for precise timing. After
>
> > each call to the timeout function, the time of the next timeout is
> > recalculated
>
> > based on the current time and the given interval"
>
> > so creating the source every time does not change much on the timing.
>
> > > One solution would be to use g_timeout_add().
>
> > >
>
> > > Another solution:
>
> > > - in main():
>
> > > replace g_timeout_add(0, vdagent_init_async_cb, agent);
>
> > > with g_idle_add(vdagent_init_async_cb, agent);
>
> > >
>
> > > - in vdagent_init_async_cb():
>
> > > if (agent->conn == NULL) {
>
> > > if (g_idle_remove_by_data(agent)) {
>
> > > g_timeout_add_seconds(1, vdagent_init_async_cb, agent);
>
> > > return G_SOURCE_REMOVE;
>
> > > }
>
> > > return G_SOURCE_CONTINUE;
>
> > > }
>
> > > This way, the source doesn't have to be recreated every single time.
>
> > >
>
> > > Jakub
>
> > >
>
> > 1 seconds is quite "human" timing thing, I don't think we need big
>
> > precision. If we really need precision I would use g_file_monitor_directory
>
> > where available to avoid entirely all the polling (which would be limited
>
> > to Unix systems without inotify or other GLib compatible implementations).
>
> > This way you can detect more or less precisely when the unix socket is
>
> > created/modified in the directory without having to check every second.
>
> > The g_idle solution should work and avoid creating the source every time,
>
> > only drawback is that adding another g_idle in a possible future is
>
> > tricky and prone to bugs as potentially the g_idle_remove_by_data would
>
> > remove the wrong source.
>
> > Frediano
>
> That's true, however, I don't think we will add another idle function any
> time soon.
That's the problem. Not soon but in a year or more people could add a g_idle call and
strangely in some corner cases the callback won't be called and will have to spend time
to debug the reason till will find the g_idle_remove_by_data deleting its g_idle source.
> It was just a note, I think we could leave it as it is,
> if nobody else complains.
> Thanks,
> Jakub
Frediano
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171018/8df92223/attachment-0001.html>
More information about the Spice-devel
mailing list