[PATCH wayland] shm: Add wl_shm_buffer get/put functions

[Derek Foreman]

On 04/10/15 10:49 AM, Jasper St. Pierre wrote:
> I imagine get/put is named after the kernel style. I typically see
> ref/unref for userspace names (or ref/destroy, but nobody likes that).

Yup, sorry.

> On Sun, Oct 4, 2015 at 8:34 AM, Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>> 2015-07-18 0:30 GMT+03:00 Derek Foreman <derekf at osg.samsung.com>:
>>> Sometimes the compositor wants to make sure a shm pool doesn't disappear
>>> out from under it.
>>>
>>> For example, in Enlightenment, rendering happens in a separate thread
>>> while the main thread can still dispatch events.  If a client is destroyed
>>> during rendering, all its resources are cleaned up and its shm pools are
>>> unmapped.  This causes the rendering thread to segfault.
>>>
>>> This patch adds a way for the compositor to increment the refcount of the
>>> shm pool so it can't disappear, and decrement it when it's finished.
>>
>> I don't like much the names of these new functions, i wouldn't expect
>> a function named get_something() to have side effects. Also,
>> wl_shm_buffer_put_pool() doesn't take a wl_shm_buffer, but the
>> wl_shm_pool.
>> What about wl_shm_buffer_ref_pool(buffer) and
>> wl_shm_pool_deref(pool)/wl_shm_buffer_deref_pool(buffer)?

In my intended use case, it's possible that the buffer will have been
destroyed before I want to release the shm pool reference.

I'll concede the asymmetry is ugly, but I think
wl_shm_buffer_deref_pool(buffer) probably shouldn't exist - I think any
time you actually want to use it you're almost certainly going to end up
with a use after free bug.

I'm ok with wl_shm_buffer_ref_pool(buffer) which returns a pool, and
wl_shm_pool_unref(pool) to release it.

(Jasper's right - unref seems to be the common name, and it's the one we
use in wayland/weston all over the place)


I'll do that in a v2 that also includes some doxy comments.

>>
>> --
>> Giulio
>>
>>>
>>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>>> ---
>>>  src/wayland-server-core.h |  7 +++++++
>>>  src/wayland-shm.c         | 15 +++++++++++++++
>>>  2 files changed, 22 insertions(+)
>>>
>>> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
>>> index e605432..a4a04fc 100644
>>> --- a/src/wayland-server-core.h
>>> +++ b/src/wayland-server-core.h
>>> @@ -362,6 +362,7 @@ wl_resource_get_destroy_listener(struct wl_resource *resource,
>>>              resource = tmp,                                                    \
>>>              tmp = wl_resource_from_link(wl_resource_get_link(resource)->next))
>>>
>>> +struct wl_shm_pool;
>>>  struct wl_shm_buffer;
>>>
>>>  void
>>> @@ -388,6 +389,12 @@ wl_shm_buffer_get_width(struct wl_shm_buffer *buffer);
>>>  int32_t
>>>  wl_shm_buffer_get_height(struct wl_shm_buffer *buffer);
>>>
>>> +struct wl_shm_pool *
>>> +wl_shm_buffer_get_pool(struct wl_shm_buffer *buffer);
>>> +
>>> +void
>>> +wl_shm_buffer_put_pool(struct wl_shm_pool *pool);
>>> +
>>>  int
>>>  wl_display_init_shm(struct wl_display *display);
>>>
>>> diff --git a/src/wayland-shm.c b/src/wayland-shm.c
>>> index 5c419fa..48b5140 100644
>>> --- a/src/wayland-shm.c
>>> +++ b/src/wayland-shm.c
>>> @@ -412,6 +412,21 @@ wl_shm_buffer_get_height(struct wl_shm_buffer *buffer)
>>>         return buffer->height;
>>>  }
>>>
>>> +WL_EXPORT struct wl_shm_pool *
>>> +wl_shm_buffer_get_pool(struct wl_shm_buffer *buffer)
>>> +{
>>> +       assert(buffer->pool->refcount);
>>> +
>>> +       buffer->pool->refcount++;
>>> +       return buffer->pool;
>>> +}
>>> +
>>> +WL_EXPORT void
>>> +wl_shm_buffer_put_pool(struct wl_shm_pool *pool)
>>> +{
>>> +       shm_pool_unref(pool);
>>> +}
>>> +
>>>  static void
>>>  reraise_sigbus(void)
>>>  {
>>> --
>>> 2.1.4
>>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> 
> 
>