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

Submitted by Frediano Ziglio on Dec. 1, 2016, 11:09 a.m.

Details

Message ID 6b5d799b62498f2c93aca6a5cca3860cfbc4bfb2.1480590563.git-series.fziglio@redhat.com
State New
Headers show
Series "Start cleaning objects on destruction" ( rev: 5 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Dec. 1, 2016, 11:09 a.m.
Allows to destroy a QXL object

Signed-off-by: Frediano Ziglio <fziglio@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(-)

Patch hide | download patch | download mbox

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);
+    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

Comments

On Thu, 2016-12-01 at 11:09 +0000, Frediano Ziglio wrote:
> Allows to destroy a QXL object
> 
> Signed-off-by: Frediano Ziglio <fziglio@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
> 
> On Thu, 2016-12-01 at 11:09 +0000, Frediano Ziglio wrote:
> > Allows to destroy a QXL object
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@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?
>

red_worker_free calls pthread_join to make sure consistency.
 
> > +    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
> 

Frediano
On Fri, 2016-12-02 at 03:35 -0500, Frediano Ziglio wrote:
> > 
> > 
> > On Thu, 2016-12-01 at 11:09 +0000, Frediano Ziglio wrote:
> > > 
> > > Allows to destroy a QXL object
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio@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?
> > 
> 
> red_worker_free calls pthread_join to make sure consistency.

Hmm, somehow I missed that. Thanks. Is it worth asserting or warning if
red_worker_free() is called from the worker thread?

Acked-by: Jonathon Jongsma <jjongsma@redhat.com>


>  
> > 
> > > 
> > > +    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
> > 
> 
> Frediano
> 
> On Fri, 2016-12-02 at 03:35 -0500, Frediano Ziglio wrote:
> > > 
> > > 
> > > On Thu, 2016-12-01 at 11:09 +0000, Frediano Ziglio wrote:
> > > > 
> > > > Allows to destroy a QXL object
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio@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?
> > > 
> > 
> > red_worker_free calls pthread_join to make sure consistency.
> 
> Hmm, somehow I missed that. Thanks. Is it worth asserting or warning if
> red_worker_free() is called from the worker thread?
> 
> Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
> 

Maybe some comments would be better. It's true that RedQxl and RedWorker
are quite close friends and only RedQxl should use RedWorker.
I'll try to write the comment:

> 
> >  
> > > 
> > > > 
> > > > +    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);
> > > > +}
> > > > +

/*
 * Free the worker thread. This function should be called by RedQxl
 * after sending a RED_WORKER_MESSAGE_CLOSE_WORKER message;
 * failing to do so will cause a deadlock.
 */

> > > > +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
> > > 
> > 

Frediano
On Fri, 2016-12-02 at 10:51 -0500, Frediano Ziglio wrote:
> > 
> > 
> > On Fri, 2016-12-02 at 03:35 -0500, Frediano Ziglio wrote:
> > > 
> > > > 
> > > > 
> > > > 
> > > > On Thu, 2016-12-01 at 11:09 +0000, Frediano Ziglio wrote:
> > > > > 
> > > > > 
> > > > > Allows to destroy a QXL object
> > > > > 
> > > > > Signed-off-by: Frediano Ziglio <fziglio@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?
> > > > 
> > > 
> > > red_worker_free calls pthread_join to make sure consistency.
> > 
> > Hmm, somehow I missed that. Thanks. Is it worth asserting or
> > warning if
> > red_worker_free() is called from the worker thread?
> > 
> > Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
> > 
> 
> Maybe some comments would be better. It's true that RedQxl and
> RedWorker
> are quite close friends and only RedQxl should use RedWorker.
> I'll try to write the comment:
> 
> > 
> > 
> > > 
> > >  
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > +    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);
> > > > > +}
> > > > > +
> 
> /*
>  * Free the worker thread. This function should be called by RedQxl
>  * after sending a RED_WORKER_MESSAGE_CLOSE_WORKER message;
>  * failing to do so will cause a deadlock.
>  */

Looks fine to me.

Jonathon


> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > +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
> > > > 
> > > 
> 
> Frediano