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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel