[PATCH wayland] server: Don't expose wl_display as a global

[Pekka Paalanen]

On Thu,  7 Aug 2014 09:55:49 -0400
"Jasper St. Pierre" <jstpierre at 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.


Thanks,
pq