[PATCH v2] server: add wl_display_destroy_clients()

[Pekka Paalanen]

On Wed, 13 Dec 2017 06:59:10 -0500
Simon Ser <contact at emersion.fr> wrote:

> >-------- Original Message --------
> >Subject: Re: [PATCH v2] server: add wl_display_destroy_clients()
> >Local Time: December 13, 2017 12:11 PM
> >UTC Time: December 13, 2017 11:11 AM
> >From: ppaalanen at gmail.com
> >To: emersion <contact at emersion.fr>
> >wayland-devel at lists.freedesktop.org
> >
> >On Wed, 13 Dec 2017 11:51:19 +0100
> > emersion contact at emersion.fr wrote:  
> >>Bug [1] reported that wl_display_destroy() doesn't destroy clients, so
> >> client socket file descriptors are being kept open until the compositor
> >> process exits.
> >>Patch [2] proposed to destroy clients in wl_display_destroy(). The
> >> patch was not accepted because doing so changes the ABI.
> >>Thus, a new wl_display_destroy_clients() function is added in this
> >> patch. It should be called by compositors right before
> >> wl_display_destroy().
> >>[1] https://bugs.freedesktop.org/show_bug.cgi?id=99142
> >> [2] https://patchwork.freedesktop.org/patch/128832/
> >>Signed-off-by: emersion contact at emersion.fr

Hi,

ah, your real name is Simon Ser? We need it in the Signed-off-by tag
for the tag to be valid, do you mind if I fix it while eventually landing
this patch?

The Linux kernel patch submission guidelines say it's required, nick
names won't do.

> >>
> >>v2: change commit message, add docs, use a safer approach when iterating
> >> over the client list
> >>src/wayland-server-core.h |  3 +++
> >> src/wayland-server.c      | 38 ++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 41 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..33df413 100644
> >> --- a/src/wayland-server.c
> >> +++ b/src/wayland-server.c
> >> @@ -1264,6 +1264,44 @@ wl_display_flush_clients(struct wl_display *display)
> >> }
> >> }
> >>+/** Destroy all clients connected to the display
> >> -  - 
> >> -  - \param display The display object
> >> -  - 
> >> -  - This function should be called right before wl_display_destroy() to ensure
> >> -  - all client resources are closed properly. Destroying a client from within
> >> -  - wl_display_destroy_clients() is safe, but creating one will leak resources
> >> -  - and raise a warning.
> >> -  - 
> >> -  - \memberof wl_display
> >> - */
> >> +WL_EXPORT void
> >> +wl_display_destroy_clients(struct wl_display *display)
> >> +{
> >> - struct wl_list tmp_client_list, *pos;
> >> - struct wl_client *client;
> >> - 
> >> - /* Move the whole client list to a temporary head because some new clients
> >> -  - might be added to the original head. */
> >> - wl_list_init(&tmp_client_list);
> >> - wl_list_insert_list(&tmp_client_list, &display->client_list);
> >> - wl_list_init(&display->client_list);
> >> - 
> >> - /* wl_list_for_each_safe isn't enough here: it fails if the next client is
> >> -  - destroyed by the destroy handler of the current one. */
> >> - while (!wl_list_empty(&tmp_client_list)) {
> >> - pos = tmp_client_list.next;
> >>
> >> - client = wl_container_of(pos, client, link);
> >>
> >> - 
> >> - wl_client_destroy(client);
> >>
> >> - }
> >> - 
> >> - 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");
> >>
> >> - }
> >> +}
> >> - 
> >>static int
> >> socket_data(int fd, uint32_t mask, void *data)
> >> {  
> >
> > Hi,
> >
> > very good!
> >
> > Reviewed-by: Pekka Paalanen pekka.paalanen at collabora.co.uk  
> 
> Thanks for your review!
> 
> > Do you have your compositor now calling this without issues?  
> 
> Yes! I've tested the new function and it works properly. Here's the patch:
> https://github.com/emersion/wlroots/commit/71d344cc616021a1d1ed6e075f8c7e0d15b0deb0

Cool.

> > It would be awesome to have a test in the libwayland test suite calling
> > this function and checking a client gets destroyed, especially as we
> > don't have Weston using this yet.  
> 
> I'll try to make another patch for this.

Appreciated.


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/d95d43f7/attachment.sig>