On 16/10/15 09:04 AM, Giulio Camuffo wrote: > 2015-10-16 16:46 GMT+03:00 Pekka Paalanen <[email protected]>: >> On Sun, 4 Oct 2015 13:46:03 +0300 >> Giulio Camuffo <[email protected]> wrote: >> >>> 2015-06-26 19:34 GMT+03:00 Derek Foreman <[email protected]>: >>>> 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 <[email protected]> >>>> --- >>>> >>>> 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 <[email protected]> >> >> 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 _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
