On Thu, 25 Jun 2015 16:26:06 -0500
Derek Foreman <[email protected]> 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 <[email protected]>
> ---
>  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 <[email protected]>

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
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to