[Spice-devel] [PATCH v5 7/8] vdagent: Use GMainLoop
Jakub Janků
janku.jakub.jj at gmail.com
Wed Oct 18 07:42:11 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.
It was just a note, I think we could leave it as it is,
if nobody else complains.
Thanks,
Jakub
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171018/47bc7bf2/attachment-0001.html>
More information about the Spice-devel
mailing list