[PATCH wayland 1/3 v2] shm: Deprecate wl_shm_buffer_create()

[Derek Foreman]

On 16/10/15 09:04 AM, Giulio Camuffo wrote:
> 2015-10-16 16:46 GMT+03:00 Pekka Paalanen <ppaalanen at gmail.com>:
>> On Sun, 4 Oct 2015 13:46:03 +0300
>> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>>
>>> 2015-06-26 19:34 GMT+03:00 Derek Foreman <derekf at osg.samsung.com>:
>>>> 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 WL_DEPRECATED
>>>> in the header.
>>>>
>>>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>>>> ---
>>>>
>>>> This time using WL_DEPRECATED
>>>>
>>>>  src/wayland-server-core.h |  2 +-
>>>>  src/wayland-shm.c         | 29 +----------------------------
>>>>  2 files changed, 2 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
>>>> index e605432..dc87168 100644
>>>> --- a/src/wayland-server-core.h
>>>> +++ b/src/wayland-server-core.h
>>>> @@ -397,7 +397,7 @@ 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) WL_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();
>>>
>>>
>>> If we abort() here we make the function unusable and fatal and i'd
>>> argue that it may be better to just remove it instead, because no
>>> function is better than a pretend function that will kill you without
>>> arguing.
>>
>> We can't remove it, because that is a major ABI break. Not when it can
>> be called without a crash at least. I suspect we'll have to keep the
>> stub forever.
> 
> Imho making it abort() is an API break as well, in a sense, and a
> worse one, because it's at runtime instead of at compile time. The
> symbol is still there but any attempt at using it will punish you
> hard, so in a way it's like if it wasn't there, but you don't have the
> compiler complaining hard enough when you use it.

Trying really hard to find a reason to disagree...  It's theoretically
possible for a library to test for the presence of the function (dlopen,
assign a function pointer) but never actually use it.

I think EFL does some clever tricks similar to this for some libraries.

So a symbol that used to be present disappearing could be a break, even
if the function is never actually called.

Maybe someone else has a more likely example of how removing this
utterly broken function would break a functional program. :)

> 
> --
> Giulio
> 
>>
>>> Maybe it's better to make it return NULL instead?
>>
>> Yeah, that would be good enough.
>>
>> NULL or abort(),
>> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>
>> Patch 2 needs a rewrite if you change anything here, but consider it
>> Acked-by me.
>>
>> Patch 3 is Acked-by me too, I don't care too much if it's abort(),
>> assert(), return NULL or all of them. If we hit a NULL pool, there is
>> guaranteed a bug somewhere, so hopefully that would be caught ASAP
>> somehow.
>>
>>
>> Thanks,
>> pq