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/ _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel