[Spice-devel] [PATCH 1/9] Use spice_server_destroy() at exit

[Jonathon Jongsma]

On Mon, 2016-10-31 at 10:44 -0400, Frediano Ziglio wrote:
> > 
> > 
> > The destructor function was calling reds_cleanup() for each server
> > at
> > application exit. But reds_cleanup() only cleans up a couple of
> > statistics-related variables. We should actually free the servers
> > by
> > calling spice_server_destroy(). This left reds_cleanup() unused, so
> > the
> > contents of this function were moved inside spice_server_destroy().
> > ---
> >  server/reds.c | 30 ++++++++++++------------------
> >  1 file changed, 12 insertions(+), 18 deletions(-)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index eeb6010..c03ed18 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -2904,27 +2904,14 @@ static int reds_init_ssl(RedsState *reds)
> >      return 0;
> >  }
> >  
> > -static void reds_cleanup(RedsState *reds)
> > -{
> > -#ifdef RED_STATISTICS
> > -    if (reds->stat_shm_name) {
> > -        shm_unlink(reds->stat_shm_name);
> > -        free(reds->stat_shm_name);
> > -        reds->stat_shm_name = NULL;
> > -    }
> > -#endif
> > -}
> > -
> >  SPICE_DESTRUCTOR_FUNC(reds_exit)
> >  {
> > -    GList *l;
> > +    GListIter iter;
> > +    RedsState *reds;
> >  
> > -    pthread_mutex_lock(&global_reds_lock);
> > -    for (l = servers; l != NULL; l = l->next) {
> > -        RedsState *reds = l->data;
> > -        reds_cleanup(reds);
> > +    GLIST_FOREACH(servers, iter, RedsState, reds) {
> > +        spice_server_destroy(reds);
> >      }
> > -    pthread_mutex_unlock(&global_reds_lock);
> >  }
> >  
> >  static inline void on_activating_ticketing(RedsState *reds)
> > @@ -3695,7 +3682,14 @@ SPICE_GNUC_VISIBLE void
> > spice_server_destroy(SpiceServer *reds)
> >      if (reds->main_channel) {
> >          main_channel_close(reds->main_channel);
> >      }
> > -    reds_cleanup(reds);
> > +
> > +#ifdef RED_STATISTICS
> > +    if (reds->stat_shm_name) {
> > +        shm_unlink(reds->stat_shm_name);
> > +        free(reds->stat_shm_name);
> > +        reds->stat_shm_name = NULL;
> > +    }
> > +#endif
> >  
> >      /* remove the server from the list of servers so that we don't
> > attempt
> >      to
> >       * free it again at exit */
> 
> I think the big problem with this patch and following about
> cleanup is how the API should behave.
> Usually if there are a XXXXX_new and a XXXXX_destroy IMHO should be
> up to the user of the library to call the destroy.
> What happens if tomorrow Qemu decides to call XXXXXX_destroy?

Qemu can call the destroy function right now regardless of whether this
patch is applied or not. This patch doesn't really change
spice_server_destroy() much at all.

> Actually it seems that even Qemu doesn't do much cleanup.
> However for instance Qemu retains some audio pointers and free them
> during an atexit callback.
> If we decide to free everything on reds_exit we need to free
> everything
> in the right order, for instance all channel, all clients, RedWorker
> thread, the dispatcher and so on. 

Sure, but now that we have GObjects that hold references on other
objects, it shouldn't be all that difficult to get this right.

> Currently we are leaving the RedWorker
> thread running that potentially will access RedsState already freed.

How can a RedWorker thread be running after the process exited?

> So surely this patch is a no go for me as we already know that it's
> racy
> and leads to crashes depending on the system load.

How do we know this? I guess I don't quite understand how this patch
makes anything worse than the current situation.

> On how to do it (and back on API behavior) what happens if Qemu wants
> to
> keep an interface (think about spice_add_interface) after a possible
> spice_server_destroy is called (for instance the audio at atexit
> time)?
> An implementation could be that users of RedsState structure will
> retain
> a reference (that is adding a reference counter to RedsState) so you
> can
> safely call spice_server_destroy before destroying any related
> interface.
> Note: the destructor functions are called after the atexit ones,
> unless
> the library (spice-server in this case) is dynamically unloaded
> before
> calling exit (which is not Qemu case).
> 
> Frediano