[PATCH wayland] shm: Deprecate wl_shm_buffer_create()

[Derek Foreman]

On 26/06/15 01:29 AM, Pekka Paalanen wrote:
> On Thu, 25 Jun 2015 16:26:06 -0500
> Derek Foreman <derekf at osg.samsung.com> 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 <derekf at osg.samsung.com>
>> ---
>>  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 <pekka.paalanen at collabora.co.uk>

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.