On 26/06/15 01:29 AM, Pekka Paalanen wrote: > 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]>
Thanks, fixed. However, existing deprecated functions all appear to have an extra prototype directly above their declaration in the .c files, should I mimic this style? > 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. Did that, but... is there really any buffer->pool can ever be NULL? Isn't the only way left to create a shm buffer through shm_pool_create_buffer()? If pool is NULL at that point we'd segfault anyway, and destroying the pool out from under the buffer won't actually set the buffer's pool pointer to NULL. > 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. There's only one check left, I think, in destroy_buffer(). If it can never actually be NULL (see above :) then I'll just pull the test - but if I'm wrong and NULL is a possible but broken condition I'd rather just add an assert() but leave the test. _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
