[PATCH] server: add wl_display_destroy_clients

[Pekka Paalanen]

On Mon, 11 Dec 2017 22:56:32 +0100
emersion <contact at emersion.fr> wrote:

> wl_display_destroy doesn't destroy clients, so client socket file
> descriptors are being kept open until the compositor process exits.
> 
> To maintain ABI compatibility, we cannot destroy clients in
> wl_display_destroy. Thus, a new wl_display_destroy_clients functions
> is added and should be called by compositors right before
> wl_display_destroy.
> 
> See https://patchwork.freedesktop.org/patch/128832/
> 
> Signed-off-by: emersion <contact at emersion.fr>

Hi,

thanks for this patch, this implements the idea I suggested in the
review comments of the earlier proposal.

Would be good to mention
https://bugs.freedesktop.org/show_bug.cgi?id=99142
in the commit message. Please wrap the commit message lines.

> ---
>  src/wayland-server-core.h |  3 +++
>  src/wayland-server.c      | 22 ++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index fd458c5..2e725d9 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -214,6 +214,9 @@ wl_display_run(struct wl_display *display);
>  void
>  wl_display_flush_clients(struct wl_display *display);
>  
> +void
> +wl_display_destroy_clients(struct wl_display *display);
> +
>  struct wl_client;
>  
>  typedef void (*wl_global_bind_func_t)(struct wl_client *client, void *data,
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 82a3b01..7f24ef1 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1264,6 +1264,28 @@ wl_display_flush_clients(struct wl_display *display)
>  	}
>  }
>  

Documentation is missing.

> +WL_EXPORT void
> +wl_display_destroy_clients(struct wl_display *display)
> +{
> +	struct wl_list tmp_client_list;
> +	struct wl_client *client, *next;
> +
> +	// Move the whole client list to a temporary head because some new clients
> +	// might be added to the original head

Use old style C comments /* */ please.

> +	wl_list_init(&tmp_client_list);
> +	wl_list_insert_list(&tmp_client_list, &display->client_list);
> +	wl_list_init(&display->client_list);

Very good.

> +
> +	wl_list_for_each_safe(client, next, &tmp_client_list, link) {
> +		wl_client_destroy(client);
> +	}

I wonder if we should be even more paranoid here. This loop is not safe
if the callbacks from destroying this wl_client already destroy the
next wl_client. What would be guaranteed safe here is to loop while the
temporary list is not empty, and always taking the first entry. An
example is in wl_priv_signal_emit().

> +
> +	if (!wl_list_empty(&display->client_list)) {
> +		wl_log("wl_display_destroy_clients: cannot destroy all clients because "
> +			   "new ones were created by destroy callbacks\n");
> +	}

Good.

> +}
> +
>  static int
>  socket_data(int fd, uint32_t mask, void *data)
>  {

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171213/2883acdb/attachment-0001.sig>