[PATCH wayland] shm: Deprecate wl_shm_buffer_create()

[Pekka Paalanen]

On Thu, 25 Jun 2015 16:26:06 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> From irc:
> <pq> it creates a wl_buffer object in a way that no client can ever
>      access the storage.
> 
> So, let's replace it with abort(); and mark it with attribute
> deprecated in the header.
> 
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
>  src/wayland-server-core.h |  3 ++-
>  src/wayland-shm.c         | 29 +----------------------------
>  2 files changed, 3 insertions(+), 29 deletions(-)
> 
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index e605432..351206b 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -397,7 +397,8 @@ wl_display_add_shm_format(struct wl_display *display, uint32_t format);
>  struct wl_shm_buffer *
>  wl_shm_buffer_create(struct wl_client *client,
>  		     uint32_t id, int32_t width, int32_t height,
> -		     int32_t stride, uint32_t format);
> +		     int32_t stride, uint32_t format)
> +	__attribute__ ((deprecated));

We have WL_DEPRECATED for this, and #ifndef WL_HIDE_DEPRECATED.

>  
>  void wl_log_set_handler_server(wl_log_func_t handler);
>  
> diff --git a/src/wayland-shm.c b/src/wayland-shm.c
> index 5c419fa..a9a0b76 100644
> --- a/src/wayland-shm.c
> +++ b/src/wayland-shm.c
> @@ -319,34 +319,7 @@ wl_shm_buffer_create(struct wl_client *client,
>  		     uint32_t id, int32_t width, int32_t height,
>  		     int32_t stride, uint32_t format)
>  {
> -	struct wl_shm_buffer *buffer;
> -
> -	if (!format_is_supported(client, format))
> -		return NULL;
> -
> -	buffer = malloc(sizeof *buffer + stride * height);
> -	if (buffer == NULL)
> -		return NULL;
> -
> -	buffer->width = width;
> -	buffer->height = height;
> -	buffer->format = format;
> -	buffer->stride = stride;
> -	buffer->offset = 0;
> -	buffer->pool = NULL;
> -
> -	buffer->resource =
> -		wl_resource_create(client, &wl_buffer_interface, 1, id);
> -	if (buffer->resource == NULL) {
> -		free(buffer);
> -		return NULL;
> -	}
> -
> -	wl_resource_set_implementation(buffer->resource,
> -				       &shm_buffer_interface,
> -				       buffer, destroy_buffer);
> -
> -	return buffer;
> +	abort();
>  }
>  
>  WL_EXPORT struct wl_shm_buffer *

With that one thing fixed:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

A follow-up could fix wl_shm_buffer_get_data() to assert(buffer->pool),
and return NULL if !buffer->pool, I think. The 'return buffer + 1;'
must be extremely confusing to a reader, mildly put.

I suspect all remaining 'if (buffer->pool)' checks should be
unnecessary. If the pool is gone, there is no storage to access, which
means we should never truely destroy the pool while there are wl_buffers
referencing it.


Thanks,
pq