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

[Frediano Ziglio]

> 
> 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?
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. Currently we are leaving the RedWorker
thread running that potentially will access RedsState already freed.
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.
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