[PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry

[Emil Velikov]

On 5 June 2018 at 18:28, Olivier Fourdan <ofourdan at redhat.com> wrote:
> Hi Emil,
>
> Many thanks for your detailed review!
>
> On Tue, Jun 5, 2018 at 12:37 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> Hi Olivier,
>>
>> There's a handful of mostly trivial suggestions below. The idea itself seems
>> reasonable IMHO. One gripe is that we're 'leaking' twice as much as before.
>>
>> Namely: even if the current backend cleans-up after itself (it some cases it
>> does not), the other backend 'leaks'. Not sure if/how much we should care in
>> that case.
>>
>> Was skimming if we cannot move more init_egl/init_screen tripping points
>> and I've noticed a few gotchas/bugs. None of which are requirement for the
>> series, although great to have.
>>
>>
>> xwl_glamor_gbm_init_egl
>>  - if !render_node error path is leaking
>
> humm, not sure, leaking what? we haven't allocated anything yet, have we?
> But anyhow, it's unralted to this refactoring I think.
>
You are right, this and the other bits listed just below are preexisting.
Thanks for tackling them.

The following are set during init_registry (wl_drm path) and
effectively leaked. Then again, there's plenty of small bits like that
throughout the code base.
I would _not_ bother with them bth.

::device_name -> strdup(device/node name)
::drm_fd -> open(^^)
::drm -> wl_registry_bind


>>> @@ -119,8 +134,8 @@ xwl_glamor_allow_commits(struct xwl_window *xwl_window)
>>>  {
>>>      struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
>>>
>>> -    if (xwl_screen->egl_backend.allow_commits)
>>> -        return xwl_screen->egl_backend.allow_commits(xwl_window);
>>> +    if (xwl_screen->egl_backend && xwl_screen->egl_backend->allow_commits)
>> Why the NULL check? Unless I'm missing something that will never happen.
>
> We can have no “egl_backend” set at all if “-shm” was specified.
>
> Either we keep that test here or we need to check for
> "xwl_screen->glamor" in xwl_screen_post_damage(), I'll change for the
> later for clarity.
>
Ah, yes. Thanks.

-Emil