Re: [Spice-devel] [PATCH v3 1/7] Add red_qxl_destroy function
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 +, Frediano Ziglio wrote: > > > > > > > > > > > > > > > Allows to destroy a QXL object > > > > > > > > > > Signed-off-by: Frediano Ziglio> > > > > --- > > > > > 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, > > > > > _cursor_cbs, > > > > > +qxl_state->worker = red_worker_new(qxl, > > > > > _cursor_cbs, > > > > > _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, > > > > > +); > > > > > +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 > > > > 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(_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 >
Re: [Spice-devel] [PATCH v3 1/7] Add red_qxl_destroy function
> > On Fri, 2016-12-02 at 03:35 -0500, Frediano Ziglio wrote: > > > > > > > > > On Thu, 2016-12-01 at 11:09 +, Frediano Ziglio wrote: > > > > > > > > Allows to destroy a QXL object > > > > > > > > Signed-off-by: Frediano Ziglio> > > > --- > > > > 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, _cursor_cbs, > > > > +qxl_state->worker = red_worker_new(qxl, _cursor_cbs, > > > > _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, > > > > +); > > > > +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 > 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(_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- > > > > > > > > > >
Re: [Spice-devel] [PATCH v3 1/7] Add red_qxl_destroy function
On Fri, 2016-12-02 at 03:35 -0500, Frediano Ziglio wrote: > > > > > > On Thu, 2016-12-01 at 11:09 +, Frediano Ziglio wrote: > > > > > > Allows to destroy a QXL object > > > > > > Signed-off-by: Frediano Ziglio> > > --- > > > 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, _cursor_cbs, > > > +qxl_state->worker = red_worker_new(qxl, _cursor_cbs, > > > _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, > > > +); > > > +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 > > > > > > > > > +g_object_unref(qxl_state->dispatcher); > > > +/* this must be done after calling red_worker_free */ > > > +qxl->st = NULL; > > > +pthread_mutex_destroy(_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(>core, worker- > > > > > > > > dispatch_watch); > > > +} > > > + > > > +
Re: [Spice-devel] [PATCH v3 1/7] Add red_qxl_destroy function
> > On Thu, 2016-12-01 at 11:09 +, Frediano Ziglio wrote: > > Allows to destroy a QXL object > > > > Signed-off-by: Frediano Ziglio> > --- > > 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, _cursor_cbs, > > +qxl_state->worker = red_worker_new(qxl, _cursor_cbs, > > _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, > > +); > > +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(_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(>core, worker- > > >dispatch_watch); > > +} > > + > > +g_main_context_unref(worker->core.main_context); > > + > > +if (worker->record) { > > +red_record_free(worker->record); > > +} > > +memslot_info_destroy(>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