[Spice-devel] [PATCH v3 1/7] Add red_qxl_destroy function

[Jonathon Jongsma]

On Thu, 2016-12-01 at 11:09 +0000, Frediano Ziglio wrote:
> Allows to destroy a QXL object
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-qxl.c    | 25 ++++++++++++++++++++++---
>  server/red-qxl.h    |  1 +
>  server/red-worker.c | 35 ++++++++++++++++++++++++++++++++++-
>  server/red-worker.h |  1 +
>  4 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 19cff95..de0546f 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -58,6 +58,7 @@ struct QXLState {
>      QXLDevSurfaceCreate surface_create;
>      unsigned int max_monitors;
>      RedsState *reds;
> +    RedWorker *worker;
>  
>      pthread_mutex_t scanout_mutex;
>      SpiceMsgDisplayGlScanoutUnix scanout;
> @@ -1006,11 +1007,29 @@ void red_qxl_init(RedsState *reds,
> QXLInstance *qxl)
>      client_display_cbs.disconnect = red_qxl_disconnect_display_peer;
>      client_display_cbs.migrate = red_qxl_display_migrate;
>  
> -    // TODO: reference and free
> -    RedWorker *worker = red_worker_new(qxl, &client_cursor_cbs,
> +    qxl_state->worker = red_worker_new(qxl, &client_cursor_cbs,
>                                         &client_display_cbs);
>  
> -    red_worker_run(worker);
> +    red_worker_run(qxl_state->worker);
> +}
> +
> +void red_qxl_destroy(QXLInstance *qxl)
> +{
> +    spice_return_if_fail(qxl->st != NULL && qxl->st->dispatcher !=
> NULL);
> +
> +    QXLState *qxl_state = qxl->st;
> +
> +    /* send message to close thread */
> +    RedWorkerMessageClose message;
> +    dispatcher_send_message(qxl_state->dispatcher,
> +                            RED_WORKER_MESSAGE_CLOSE_WORKER,
> +                            &message);
> +    red_worker_free(qxl_state->worker);

Is this safe? You're sending a CLOSE_WORKER message and then freeing
the worker. But the CLOSE_WORKER handler (handle_dev_close()) also uses
the worker (via 'opaque'). I don't think we can guarantee that the
RedWorker's handler has executed already, can we?

> +    g_object_unref(qxl_state->dispatcher);
> +    /* this must be done after calling red_worker_free */
> +    qxl->st = NULL;
> +    pthread_mutex_destroy(&qxl_state->scanout_mutex);
> +    free(qxl_state);
>  }
>  
>  Dispatcher *red_qxl_get_dispatcher(QXLInstance *qxl)
> diff --git a/server/red-qxl.h b/server/red-qxl.h
> index 65357b1..7743124 100644
> --- a/server/red-qxl.h
> +++ b/server/red-qxl.h
> @@ -24,6 +24,7 @@
>  typedef struct AsyncCommand AsyncCommand;
>  
>  void red_qxl_init(SpiceServer *reds, QXLInstance *qxl);
> +void red_qxl_destroy(QXLInstance *qxl);
>  
>  void red_qxl_on_ic_change(QXLInstance *qxl, SpiceImageCompression
> ic);
>  void red_qxl_on_sv_change(QXLInstance *qxl, int sv);
> diff --git a/server/red-worker.c b/server/red-worker.c
> index d699bd6..5dac0ec 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -1410,7 +1410,6 @@ static void *red_worker_main(void *arg)
>      g_main_loop_unref(loop);
>      worker->loop = NULL;
>  
> -    /* FIXME: free worker, and join threads */
>      return NULL;
>  }
>  
> @@ -1435,3 +1434,37 @@ bool red_worker_run(RedWorker *worker)
>  
>      return r == 0;
>  }
> +
> +static void red_worker_close_channel(RedChannel *channel)
> +{
> +    red_channel_reset_thread_id(channel);
> +    red_channel_destroy(channel);
> +}
> +
> +void red_worker_free(RedWorker *worker)
> +{
> +    RedsState *reds = red_qxl_get_server(worker->qxl->st);
> +
> +    /* prevent any possible future attempt to connect to new clients
> */
> +    reds_unregister_channel(reds, RED_CHANNEL(worker-
> >cursor_channel));
> +    reds_unregister_channel(reds, RED_CHANNEL(worker-
> >display_channel));
> +
> +    pthread_join(worker->thread, NULL);
> +
> +    red_worker_close_channel(RED_CHANNEL(worker->cursor_channel));
> +    worker->cursor_channel = NULL;
> +    red_worker_close_channel(RED_CHANNEL(worker->display_channel));
> +    worker->display_channel = NULL;
> +
> +    if (worker->dispatch_watch) {
> +        worker->core.watch_remove(&worker->core, worker-
> >dispatch_watch);
> +    }
> +
> +    g_main_context_unref(worker->core.main_context);
> +
> +    if (worker->record) {
> +        red_record_free(worker->record);
> +    }
> +    memslot_info_destroy(&worker->mem_slots);
> +    free(worker);
> +}
> diff --git a/server/red-worker.h b/server/red-worker.h
> index 53b92b3..d5b5a78 100644
> --- a/server/red-worker.h
> +++ b/server/red-worker.h
> @@ -29,5 +29,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>                            const ClientCbs *client_cursor_cbs,
>                            const ClientCbs *client_display_cbs);
>  bool       red_worker_run(RedWorker *worker);
> +void red_worker_free(RedWorker *worker);
>  
>  #endif