On Thu, 21 Aug 2014 09:05:48 -0700 Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Thu, Aug 21, 2014 at 4:00 AM, Pekka Paalanen <ppaala...@gmail.com> > wrote: > > > On Thu, 7 Aug 2014 09:55:49 -0400 > > "Jasper St. Pierre" <jstpie...@mecheye.net> wrote: > > > > > The idea here was that once upon a time, clients could rebind > > > wl_display to a higher version, so we offered the ability to > > > rebind it here. However, this is particularly broken. The > > > existing bind implementation actually still hardcodes version > > > numbers, and it leaks previous resources, overwriting the > > > existing one. > > > > > > The newly bound resource *also* won't have any listeners attached > > > by the client, meaning that the error and delete_id events won't > > > get delivered correctly. Unless the client poked into libwayland > > > internals, it also can't possibly set up these handlers correctly > > > either, so the client will sustain errors and leak all deleted > > > globals. > > > > > > Since this never worked correctly in the first place, we can feel > > > safe removing it. > > > --- > > > src/wayland-server.c | 28 ++++++++-------------------- > > > 1 file changed, 8 insertions(+), 20 deletions(-) > > > > > > diff --git a/src/wayland-server.c b/src/wayland-server.c > > > index 3c162d4..dc3f502 100644 > > > --- a/src/wayland-server.c > > > +++ b/src/wayland-server.c > > > @@ -380,9 +380,8 @@ wl_client_get_display(struct wl_client > > > *client) return client->display; > > > } > > > > > > -static void > > > -bind_display(struct wl_client *client, > > > - void *data, uint32_t version, uint32_t id); > > > +static int > > > +bind_display(struct wl_client *client, struct wl_display > > > *display); > > > > > > /** Create a client for the given file descriptor > > > * > > > @@ -440,9 +439,7 @@ wl_client_create(struct wl_display *display, > > > int fd) goto err_map; > > > > > > wl_signal_init(&client->destroy_signal); > > > - bind_display(client, display, 1, 1); > > > - > > > - if (!client->display_resource) > > > + if (bind_display(client, display) < 0) > > > goto err_map; > > > > > > wl_list_insert(display->client_list.prev, &client->link); > > > @@ -772,22 +769,20 @@ destroy_client_display_resource(struct > > > wl_resource > > *resource) > > > resource->client->display_resource = NULL; > > > } > > > > > > -static void > > > -bind_display(struct wl_client *client, > > > - void *data, uint32_t version, uint32_t id) > > > +static int > > > +bind_display(struct wl_client *client, struct wl_display > > > *display) { > > > - struct wl_display *display = data; > > > - > > > client->display_resource = > > > - wl_resource_create(client, &wl_display_interface, > > > 1, id); > > > + wl_resource_create(client, &wl_display_interface, > > > 1, 1); if (client->display_resource == NULL) { > > > wl_client_post_no_memory(client); > > > - return; > > > + return -1; > > > } > > > > > > wl_resource_set_implementation(client->display_resource, > > > &display_interface, display, > > > destroy_client_display_resource); > > > + return 0; > > > } > > > > > > /** Create Wayland display object. > > > @@ -831,13 +826,6 @@ wl_display_create(void) > > > > > > wl_array_init(&display->additional_shm_formats); > > > > > > - if (!wl_global_create(display, &wl_display_interface, 1, > > > - display, bind_display)) { > > > - wl_event_loop_destroy(display->loop); > > > - free(display); > > > - return NULL; > > > - } > > > - > > > return display; > > > } > > > > > > > I asked krh about this in irc, and he eventually agreed that > > exposing wl_display as a global is not so useful. > > > > I reviewed the patch, and the alleged problems the current code has, > > and I agree they are real. > > > > If we ever end up needing wl_display as a global, we can easily add > > it back. Since the implementation was indeed broken, no client > > should ever try to bind to wl_display global with version 1. > > > > Therefore, pushed with ack from Jason and R-b from me. > > > > In fact, I think this warrants this patch as a condidate for the > > stable branch. > > > > Yeah, it probably counts as a bug-fix. It does change behaviour > though, so I'm not sure I really want to call it a stable thing. > Unless, of course, re-binding wl_display could cause a crash; in > which case, push it to 1.5 --Jason Let's see if there is any fallout during the alpha period, and think about the stable branch then. Thanks, pq _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel