[Spice-devel] [PATCH spice-server] event loop: improve implementation of watches

[Frediano Ziglio]

> 
> Hi,
> 
> On Wed, Jun 19, 2019 at 04:56:41PM +0100, Frediano Ziglio wrote:
> > Avoid having to destroy and create a new GSource every time
> > we change event mask.
> > Interfaces required for this patch are available since GLib 2.36.
> > on Windows GPollFD::fd can be an HANDLE but not a socket.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/event-loop.c | 97 +++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 85 insertions(+), 12 deletions(-)
> > 
> > diff --git a/server/event-loop.c b/server/event-loop.c
> > index 80af2954f..33db4ffb0 100644
> > --- a/server/event-loop.c
> > +++ b/server/event-loop.c
> > @@ -85,14 +85,6 @@ static void timer_remove(const
> > SpiceCoreInterfaceInternal *iface,
> >      g_free(timer);
> >  }
> >  
> > -struct SpiceWatch {
> > -    GMainContext *context;
> > -    void *opaque;
> > -    GSource *source;
> > -    GIOChannel *channel;
> > -    SpiceWatchFunc func;
> > -};
> > -
> >  static GIOCondition spice_event_to_giocondition(int event_mask)
> >  {
> >      GIOCondition condition = 0;
> > @@ -117,6 +109,15 @@ static int giocondition_to_spice_event(GIOCondition
> > condition)
> >      return event;
> >  }
> >  
> > +#ifdef _WIN32
> > +struct SpiceWatch {
> > +    GMainContext *context;
> > +    void *opaque;
> > +    GSource *source;
> > +    GIOChannel *channel;
> > +    SpiceWatchFunc func;
> > +};
> > +
> >  static gboolean watch_func(GIOChannel *source, GIOCondition condition,
> >                             gpointer data)
> >  {
> > @@ -161,11 +162,7 @@ static SpiceWatch *watch_add(const
> > SpiceCoreInterfaceInternal *iface,
> >  
> >      watch = g_new0(SpiceWatch, 1);
> >      watch->context = iface->main_context;
> > -#ifndef _WIN32
> > -    watch->channel = g_io_channel_unix_new(fd);
> > -#else
> >      watch->channel = g_io_channel_win32_new_socket(fd);
> > -#endif
> >      watch->func = func;
> >      watch->opaque = opaque;
> >  
> > @@ -184,6 +181,82 @@ static void watch_remove(const
> > SpiceCoreInterfaceInternal *iface,
> >      g_free(watch);
> >  }
> >  
> > +#else
> > +
> > +struct SpiceWatch {
> > +    GSource source;
> > +    GPollFD pollfd;
> > +};
> > +
> > +static gboolean
> > +spice_watch_check(GSource *source)
> > +{
> > +    SpiceWatch *watch = SPICE_CONTAINEROF(source, SpiceWatch, source);
> > +
> > +    return watch->pollfd.events & watch->pollfd.revents;
> > +}
> > +
> > +static gboolean
> > +spice_watch_dispatch(GSource     *source,
> > +                     GSourceFunc  callback,
> > +                     gpointer     user_data)
> > +{
> > +    SpiceWatch *watch = SPICE_CONTAINEROF(source, SpiceWatch, source);
> > +    SpiceWatchFunc func = (SpiceWatchFunc)(void*) callback;
> > +
> > +    func(watch->pollfd.fd,
> > giocondition_to_spice_event(watch->pollfd.revents), user_data);
> > +    /* timer might be free after func(), don't touch */
> > +
> > +    return G_SOURCE_CONTINUE;
> > +}
> > +
> > +static GSourceFuncs spice_watch_funcs = {
> > +    .check = spice_watch_check,
> > +    .dispatch = spice_watch_dispatch,
> > +};
> > +
> > +static void watch_update_mask(const SpiceCoreInterfaceInternal *iface,
> > +                              SpiceWatch *watch, int event_mask)
> > +{
> > +    GIOCondition old_condition = watch->pollfd.events;
> > +    GIOCondition new_condition = spice_event_to_giocondition(event_mask);
> > +
> > +    watch->pollfd.events = new_condition;
> > +    if (old_condition && !new_condition) {
> > +        g_source_remove_poll(&watch->source, &watch->pollfd);
> > +    } else if (!old_condition && new_condition) {
> > +        g_source_add_poll(&watch->source, &watch->pollfd);
> 
> There is a note in the manual "Newly-written event sources should
> try to use g_source_add_unix_fd() instead of this API." which is
> present since 2.36.
> 

I'll update. Original patch was written in 2015 (at that time g_source_add_poll
was not present in the minimum glib version we supported).

> > +    }
> > +}
> > +
> > +static SpiceWatch *watch_add(const SpiceCoreInterfaceInternal *iface,
> > +                             int fd, int event_mask, SpiceWatchFunc func,
> > void *opaque)
> > +{
> > +    SpiceWatch *watch = (SpiceWatch *) g_source_new(&spice_watch_funcs,
> > sizeof(SpiceWatch));
> > +
> > +    spice_return_val_if_fail(fd != -1, NULL);
> > +    spice_return_val_if_fail(func != NULL, NULL);
> 
> If an issue in the guards, you would be leaking the GSource
> 

Not a big deal, abort() will be called so resources will be freed by
the OS.
Note that this is not a regression, same code in the old watch_add.

> > +
> > +    watch->pollfd.fd = fd;
> > +    watch->pollfd.events = 0;
> > +
> > +    g_source_set_callback(&watch->source,
> > (GSourceFunc)(void*)(SpiceWatchFunc) func, opaque, NULL);
> 
> I guess G_SOURCE_FUNC should work for casting *but* that is in
> 2.58 only :(
> 
> Potential addition to glib-compat.h ?
> 

I don't think there are so much occurrences, maximum I would add
the macro in this file in a "#ifndef G_SOURCE_FUNC".

> > +
> > +    g_source_attach(&watch->source, iface->main_context);
> > +
> > +    watch_update_mask(iface, watch, event_mask);
> > +
> > +    return watch;
> > +}
> > +
> > +static void watch_remove(const SpiceCoreInterfaceInternal *iface,
> > +                         SpiceWatch *watch)
> > +{
> 
> Not sure on situations that watch_remove() can be called actually
> but you might considering add a conditional with
> g_source_is_destroyed() to avoid unref multiple times. Perhaps
> not important.
> 

We have tests and CI in place for these situations.
They are called for instance when RedStream is passed from reds to RedChannel.

> Cheers,
> Victor
> 
> > +    g_source_destroy(&watch->source);
> > +    g_source_unref(&watch->source);
> > +}
> > +#endif
> > +
> >  const SpiceCoreInterfaceInternal event_loop_core = {
> >      .timer_add = timer_add,
> >      .timer_start = timer_start,

Frediano