[PATCH] desktop-shell: destroy data before exiting.

[Pekka Paalanen]

On Tue,  2 Oct 2012 21:12:35 -0700
"U. Artie Eoff" <ullysses.a.eoff at intel.com> wrote:

> From: "U. Artie Eoff" <ullysses.a.eoff at intel.com>
> 
> desktop-shell never returned from display_run() since it
> was essentially killed when weston exited.  To fix this,
> it is necessary to watch for EPOLLHUP in window.c so that
> toytoolkit clients will return from display_run() when
> weston quits.  This allows for clients to clean up
> as needed.
> 
> Signed-off-by: U. Artie Eoff <ullysses.a.eoff at intel.com>

Hi Artie,

I very much like this, though I didn't carefully review the patch. This
also answers Scott's questions about where things are freed. ;-)

One minor suggestion at the end.

> ---
>  clients/desktop-shell.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++
>  clients/window.c        | 21 +++++++++---
>  2 files changed, 105 insertions(+), 4 deletions(-)
> 
<...>
> diff --git a/clients/window.c b/clients/window.c
> index f3b61de..6695978 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -84,6 +84,7 @@ struct display {
>  	uint32_t serial;
>  
>  	int display_fd;
> +	uint32_t display_fd_events;
>  	uint32_t mask;
>  	struct task display_task;
>  
> @@ -3727,7 +3728,14 @@ handle_display_data(struct task *task, uint32_t events)
>  {
>  	struct display *display =
>  		container_of(task, struct display, display_task);
> -	
> +
> +	display->display_fd_events = events;
> +
> +	if (events & EPOLLERR || events & EPOLLHUP) {
> +		display_exit(display);
> +		return;
> +	}
> +
>  	wl_display_iterate(display->display, display->mask);
>  }
>  
> @@ -3751,7 +3759,8 @@ display_create(int argc, char *argv[])
>  	d->epoll_fd = os_epoll_create_cloexec();
>  	d->display_fd = wl_display_get_fd(d->display, event_mask_update, d);
>  	d->display_task.run = handle_display_data;
> -	display_watch_fd(d, d->display_fd, EPOLLIN, &d->display_task);
> +	display_watch_fd(d, d->display_fd, EPOLLIN | EPOLLERR | EPOLLHUP,
> +			 &d->display_task);
>  
>  	wl_list_init(&d->deferred_list);
>  	wl_list_init(&d->input_list);
> @@ -3815,7 +3824,8 @@ void
>  display_destroy(struct display *display)
>  {
>  	if (!wl_list_empty(&display->window_list))
> -		fprintf(stderr, "toytoolkit warning: windows exist.\n");
> +		fprintf(stderr, "toytoolkit warning: %d windows exist.\n",
> +			wl_list_length(&display->window_list));
>  
>  	if (!wl_list_empty(&display->deferred_list))
>  		fprintf(stderr, "toytoolkit warning: deferred tasks exist.\n");
> @@ -3845,7 +3855,10 @@ display_destroy(struct display *display)
>  
>  	close(display->epoll_fd);
>  
> -	wl_display_flush(display->display);
> +	if (!(display->display_fd_events & EPOLLERR) &&
> +	    !(display->display_fd_events & EPOLLHUP))
> +		wl_display_flush(display->display);

As display_exit() simply sets display->running to zero, maybe you could
replace display->running with something that can indicate "running",
"exiting", and "exiting with Wayland connection error", the last one
only triggered by EPOLLERR and EPOLLHUP. It would make this last
if-statement more obvious.

> +
>  	wl_display_disconnect(display->display);
>  	free(display);
>  }


Thanks,
pq