[Spice-devel] [PATCH vdagent v2 7/8] vdagent: Use GMainLoop

Jakub Janků janku.jakub.jj at gmail.com
Thu Oct 5 10:40:15 UTC 2017


On Thu, Oct 5, 2017, 11:20 AM Frediano Ziglio <fziglio at redhat.com> wrote:

> >
> > 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?
> >
>
> #define keep_running do_daemonize
>
> ??
>
> or you could release agent->loop and set to NULL so
> you can check it but this sounds more complicate.
>
> > >> +    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);
> > >>
>
> Just realized a nasty problem with this.
> This function register a GSource in the default main context so
> you'll have (references):
>
>   main_context -> source with x11_io_channel_cb and agent
>
> however if you have a reconnection (goto reconnect in main)
> agent if freed and created again. But the source will still
> exits with old sources. When loop will be executed potentially
> these sources will be executed using the old agent pointer
> which now is invalid. Note that we register the watch for X11
> and the timeout for initialization.
> In case udscs disconnect you'll have the loop recreating
> the agent.
> Maybe (not sure) you could use g_main_context_find_source_by_user_data
> with agent and g_main_context_default to find the sources you should
> remove (not sure if the user_data in the source is the agent).
>

I think there's an easy fix:
- both g_io_add_watch() and g_timeout_add_seconds() return a source tag (id)
- when freeing the agent, the sources can be removed with
g_source_remove(id)

We already do this in the udscs.c, have a look. I somehow forgot to add
this to the vdagent...

>
> > >>      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
> >
>
> Frediano
>

Thanks for your review,
  Jakub

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171005/874efce8/attachment-0001.html>


More information about the Spice-devel mailing list