From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35761) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R08IV-0008DD-Gd for qemu-devel@nongnu.org; Sun, 04 Sep 2011 04:43:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R08IU-0005h4-E1 for qemu-devel@nongnu.org; Sun, 04 Sep 2011 04:43:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59419) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R08IU-0005gy-75 for qemu-devel@nongnu.org; Sun, 04 Sep 2011 04:43:22 -0400 Message-ID: <4E633A37.40208@redhat.com> Date: Sun, 04 Sep 2011 11:43:35 +0300 From: Yonit Halperin MIME-Version: 1.0 References: <1314976794-31931-1-git-send-email-kraxel@redhat.com> In-Reply-To: <1314976794-31931-1-git-send-email-kraxel@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Spice-devel] [PATCH] server: don't call reds_stream_free from worker thread context List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org, spice-devel@freedesktop.org On 09/02/2011 06:19 PM, Gerd Hoffmann wrote: > reds_stream_free() may call the channel_event callback which is not > supposed to be callsed from worker thread context. This patch moves > the reds_stream_free call for the display channel from the worker to > the dispatcher to fix this issue. > > [ Note: not tested yet, against 0.8 branch, sending out for review& > comments nevertheless ] > > Signed-off-by: Gerd Hoffmann > --- > server/red_dispatcher.c | 5 +++++ > server/red_worker.c | 3 +-- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c > index f74b13e..801a575 100644 > --- a/server/red_dispatcher.c > +++ b/server/red_dispatcher.c > @@ -51,6 +51,7 @@ struct RedDispatcher { > int y_res; > int use_hardware_cursor; > RedDispatcher *next; > + RedsStream *stream; > RedWorkerMessage async_message; > pthread_mutex_t async_lock; > QXLDevSurfaceCreate surface_create; > @@ -81,6 +82,7 @@ static void red_dispatcher_set_peer(Channel *channel, RedsStream *stream, int mi > > red_printf(""); > dispatcher = (RedDispatcher *)channel->data; > + dispatcher->stream = stream; > RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_CONNECT; > write_message(dispatcher->channel,&message); > send_data(dispatcher->channel,&stream, sizeof(RedsStream *)); > @@ -93,6 +95,9 @@ static void red_dispatcher_shutdown_peer(Channel *channel) > red_printf(""); > RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_DISCONNECT; > write_message(dispatcher->channel,&message); > + read_message(dispatcher->channel,&message); > + ASSERT(message == RED_WORKER_MESSAGE_READY); > + reds_stream_free(dispatcher->stream); Hi, RED_WORKER_MESSAGE_DISPLAY_DISCONNECT is not the only place that triggers red_disconnect_channel (and as a result, reds_stream_free(dispatcher->stream)). red_disconnect_channel is called also when there is an error upon receive/send and also when timeouts related to the client occur (e.g., in flush_display_commands). We probably better make the dispatcher bi-directional, i.e., not only push messages to the worker, but also listen. Cheers, Yonit. > } > > static void red_dispatcher_migrate(Channel *channel) > diff --git a/server/red_worker.c b/server/red_worker.c > index 5f07803..f77b0f2 100644 > --- a/server/red_worker.c > +++ b/server/red_worker.c > @@ -8486,8 +8486,6 @@ static void red_disconnect_channel(RedChannel *channel) > { > channel_release_res(channel); > red_pipe_clear(channel); > - reds_stream_free(channel->stream); > - channel->stream = NULL; > channel->send_data.blocked = FALSE; > channel->send_data.size = channel->send_data.pos = 0; > spice_marshaller_reset(channel->send_data.marshaller); > @@ -10060,6 +10058,7 @@ static void handle_dev_input(EventListener *listener, uint32_t events) > case RED_WORKER_MESSAGE_CURSOR_DISCONNECT: > red_printf("cursor disconnect"); > red_disconnect_cursor((RedChannel *)worker->cursor_channel); > + write_ready = 1; > break; > case RED_WORKER_MESSAGE_CURSOR_MIGRATE: > red_printf("cursor migrate");