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

[Pekka Paalanen]

On Fri, 16 Oct 2015 09:19:57 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> 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. :)

The dynamic linker ld.so with LD_BIND_NOW, nothing to do with dlopen()
necessarily. Could be a piece of code calling this that itself is never
called.

I suspect someone (binary distributions?) might also be doing ABI
checks based on exported symbols, and if a symbol disappears, it gets
flagged as a break. I believe some projects (Mesa nowadays, right?)
actually maintain lists of exported symbols to keep ABI.

The one thing I am quite sure is that we must not remove this function.
Whether it aborts or just always returns NULL, I don't care too much.

Even if it's just to make ABI checking tools happy.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20151106/136177cd/attachment.sig>