[PATCH] weston: implement inert objects for keyboard/pointer/touch

[Hardening]

Le 20/10/2015 17:12, Bryce Harrington a écrit :
> On Tue, Oct 20, 2015 at 10:39:51AM +0200, Hardening wrote:
>> Le 20/10/2015 10:24, Bryce Harrington a écrit :
>>> On Mon, Oct 19, 2015 at 03:47:19PM +0200, David FORT wrote:
>>>> This patch implements inert objects for wl_keyboard, wl_pointer and wl_touch.
>>>> The target case is when the server has just send a capability event about a
>>>> disappearing object, and the client binds the corresponding object. We bind an
>>>> inert object: an object does nothing when it is requested. If the client behave
>>>> correctly, this object should be released when the capability event is received
>>>> and treated (calling the corresponding release request).
>>>> As a consequence, we can rely on seat->[keyboard|pointer|touch]_state to know
>>>> if the seat has the corresponding object.
>>>
>>> This is a big patch but it looks like the vast bulk is merely adding in
>>> pointer checks for keyboard, pointer, etc. everywhere and subsequent
>>> retabbing.  If you broke that refactoring step out as a preliminary
>>> patch, it may make reviewing the pertinent changes (i.e. tracking and
>>> checking state for the input devices rather than just testing the
>>> reference count) a bit easier.
>>
>> I'm not sure to understand, what should I do then ?
> 
> Create two patches, the first of which just adds all the "if
> (keyboard)..." checks, and the second (much smaller) patch which adds
> the inert_pointer_interface, inert_keyboard_interface, etc. parts which
> are what you actually need the review on.  The pointer checking is
> obviously a safe refactoring that can be landed with minimal review.
> 

Well I can do this way but the 2 patches are linked. In the current
code, once a keyboard has been created, you can't have a null
seat->keyboard_state. So in the current situation, I'm not sure the
checks are really needed .

>>> Thanks for tending to the test code too in the refactor; it would be
>>> grand to see a test case added to keyboard-test or devices-test to
>>> exercise the case of handling inert input objects.
>>>
>>
>> Such a test is already there in the existing tests. In
>> get_device_after_destroy of devices-test.c, we already exercise inert
>> objects.
> 
> Apparently it is not exercised sufficiently, because it passes already
> without your fix.
> 

Well you're right: in the current code input objects are never released,
so the calls are done on used objects without any visible side effect.
In fact, there's a side effect when calling wl_pointer.set_cursor on a
released pointer: the surface is given the mouse pointer role. Using
inert objects, you're just sure that there will be no side effect at
all. As I said It think it's also the first step to make it possible to
safely release the full seat.

> But you're right that this test case looks like a good place for you to
> add more coverage for this bug.  Does the current test pass without your
> fix because it is not making deep enough wayland calls?  I.e. does it
> need to do more than merely a wl_pointer_set_cursor call?
> 
> Also, it appears this test isn't doing the same checking for keyboard
> and touch as it does for pointer; even though internally all three are
> implemented the same way, there are three different protocol routines
> involved here (wl_seat_get_pointer, wl_seat_get_keyboard, and
> wl_seat_get_touch), any of which could present their own bugs, so proper
> test coverage should methodically check all three.
> 

+1 for testing all input objects.

Best regards.

-- 
David FORT
website: http://www.hardening-consulting.com/