[PATCH 2/6 weston] xserver: glue it with shell_surface

[Ander Conselvan de Oliveira]

On 04/03/2012 04:58 PM, Tiago Vignatti wrote:
> Signed-off-by: Tiago Vignatti<tiago.vignatti at intel.com>
> ---
>   src/Makefile.am        |    4 +++-
>   src/shell.c            |   17 +++++++++++++++--
>   src/shell.h            |   34 ++++++++++++++++++++++++++++++++++
>   src/xserver-launcher.c |   14 ++++++++++++++
>   4 files changed, 66 insertions(+), 3 deletions(-)
>   create mode 100644 src/shell.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 048e58f..afd8dcd 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -36,7 +36,8 @@ xserver_launcher_sources =			\
>   	xserver-protocol.c			\
>   	xserver-server-protocol.h		\
>   	hash.c					\
> -	hash.h
> +	hash.h					\
> +	$(desktop_shell_la_SOURCES)
>   endif

What if Weston is running with the xserver-launcher and a shell 
different that desktop-shell? I did not see any runtime check on the 
rest of the patch to make sure desktop-shell is being used.

[...]

> diff --git a/src/xserver-launcher.c b/src/xserver-launcher.c
> index 4fda42f..91f827b 100644
> --- a/src/xserver-launcher.c
> +++ b/src/xserver-launcher.c

[...]

> @@ -1529,9 +1535,17 @@ xserver_set_window_id(struct wl_client *client, struct wl_resource *resource,
>   	fprintf(stderr, "set_window_id %d for surface %p\n", id, surface);
>
>   	window->surface = (struct weston_surface *) surface;
> +	window->client = client;
>   	window->surface_destroy_listener.func = surface_destroy;
>   	wl_list_insert(surface->resource.destroy_listener_list.prev,
>   		&window->surface_destroy_listener.link);
> +
> +	/* Given that we're stealing the same client call to create
> +	 * shell_surface, the id here follows X11 convention i.e, using big
> +	 * integers (drawable.id). This is different from Wayland style but
> +	 * seems to work -- we remark that it might be dangerous though. TODO:
> +	 * create a testing for it on the protocol. */
> +	shell_get_shell_surface(client, resource, id, surface_resource);

This is all sorts of bad! You are just taking a random id from X and 
using it as an id in the client space, without the client being aware of 
it. If the client have already used this id you replace that object in 
the map. Otherwise, the client may replace this object in the map later 
on when it decides to use this id.

Also, the map is implemented as an array indexed by the id. Using a high 
id will force the array size to be reallocated to the smallest power of 
two that is bigger or equal to

	id * sizeof(struct wl_map_entry).

If you use 0 as the id, the object will be allocated in the server id 
space and you avoid all those problems.


Cheers,
Ander