On Wed, 13 Dec 2017 06:59:10 -0500 Simon Ser <[email protected]> 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: [email protected] > >To: emersion <[email protected]> > >[email protected] > > > >On Wed, 13 Dec 2017 11:51:19 +0100 > > emersion [email protected] 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 [email protected] 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 [email protected] > > 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
pgpdKY1xaHCyU.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
