[WESTON patch 2/2] tests: add tests for devices handling
Marek Chalupa
mchqwerty at gmail.com
Mon Mar 23 04:21:24 PDT 2015
Hi,
On Thu, Mar 19, 2015 at 10:51 AM, Pekka Paalanen <ppaalanen at gmail.com>
wrote:
> On Thu, 19 Mar 2015 03:38:52 -0400
> Marek Chalupa <mchqwerty at gmail.com> wrote:
>
> > Test misc races when adding/releasing devices. One of the
> > tests reveals a race that is not currently handled by Weston.
> >
> > Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> > ---
> > Makefile.am | 7 ++-
> > tests/devices-test.c | 160
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 166 insertions(+), 1 deletion(-)
> > create mode 100644 tests/devices-test.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index c509f6e..b73cf59 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -936,7 +936,8 @@ weston_tests = \
> > text.weston \
> > presentation.weston \
> > roles.weston \
> > - subsurface.weston
> > + subsurface.weston \
> > + devices.weston
> >
> >
> > AM_TESTS_ENVIRONMENT = \
> > @@ -1027,6 +1028,10 @@ button_weston_SOURCES = tests/button-test.c
> > button_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> > button_weston_LDADD = libtest-client.la
> >
> > +devices_weston_SOURCES = tests/devices-test.c
> > +devices_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> > +devices_weston_LDADD = libtest-client.la
> > +
> > text_weston_SOURCES = tests/text-test.c
> > nodist_text_weston_SOURCES = \
> > protocol/text-protocol.c \
> > diff --git a/tests/devices-test.c b/tests/devices-test.c
> > new file mode 100644
> > index 0000000..bd32674
> > --- /dev/null
> > +++ b/tests/devices-test.c
> > @@ -0,0 +1,160 @@
> > +/*
> > + * Copyright © 2015 Red Hat, Inc.
> > + *
> > + * Permission to use, copy, modify, distribute, and sell this software
> and
> > + * its documentation for any purpose is hereby granted without fee,
> provided
> > + * that the above copyright notice appear in all copies and that both
> that
> > + * copyright notice and this permission notice appear in supporting
> > + * documentation, and that the name of the copyright holders not be
> used in
> > + * advertising or publicity pertaining to distribution of the software
> > + * without specific, written prior permission. The copyright holders
> make
> > + * no representations about the suitability of this software for any
> > + * purpose. It is provided "as is" without express or implied warranty.
> > + *
> > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> > + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> > + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> > + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> > + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> > + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > + */
> > +
> > +#include "config.h"
> > +
> > +#include <string.h>
> > +#include "weston-test-client-helper.h"
> > +
> > +/* simply test if weston sends the right capabilities when
> > + * some devices are removed */
> > +TEST(seat_capabilities_test)
> > +{
> > + struct client *cl = client_create(100, 100, 100, 100);
> > + assert(cl->input->pointer);
> > + weston_test_device_release(cl->test->weston_test, "pointer");
> > + client_roundtrip(cl);
> > + /* do two roundtrips - the first makes sure the device was
> > + * released, the other that new capabilities came */
> > + client_roundtrip(cl);
>
> Hi,
>
> Why do you need two roundtrips?
>
> Are the new capabilities not sent while processing the device_release
> request?
>
Yes, they are. The capabilities are received before the wl_callback.done,
so one roundtrip is enough. My mistake. Don't know why I had thought
I need two roundtrips.
>
> weston_seat_release_pointer() calls seat_send_updated_caps() which
> calls wl_seat_send_capabilities(). This means the capability event is
> emitted before the compositor has a chance to process the
> wl_display.sync, which means the capabilities come necessarily before
> the wl_callback.done of the roundtrip.
>
> weston-test-client-helper.c also processes the capability event
> immediately.
>
> Am I missing something?
>
> When you *add a keyboard*, then you may want the double-roundtrip to
> make sure the keymap event has arrived after creating the wl_keyboard
> object.
>
> > +
> > + assert(!cl->input->pointer);
> > + assert(cl->input->keyboard);
> > + weston_test_device_release(cl->test->weston_test, "keyboard");
> > + client_roundtrip(cl);
> > +
> > + client_roundtrip(cl);
> > + assert(!cl->input->keyboard);
> > +
> > + weston_test_device_add(cl->test->weston_test, "keyboard");
> > + weston_test_device_add(cl->test->weston_test, "pointer");
> > + client_roundtrip(cl);
> > + client_roundtrip(cl);
> > +
> > + assert(cl->input->pointer);
> > + assert(cl->input->keyboard);
> > +}
> > +
> > +TEST(device_release_before_destroy)
>
> For completeness, shouldn't we also test device_release_after_destroy?
> Which is the usual sequence.
>
> weston-test-client-helper.c uses only the destroy methods, not the
> release.
>
Good idea
>
> > +{
> > + struct client *cl = client_create(100, 100, 100, 100);
> > +
> > + /* we can release pointer when we won't be using it anymore.
> > + * Do it and see what happens if the device is destroyed
> > + * right after that */
> > + wl_pointer_release(cl->input->pointer->wl_pointer);
> > + /* we must free and set to NULL the structures, otherwise
> > + * seat capabilities will double-free them */
> > + free(cl->input->pointer);
> > + cl->input->pointer = NULL;
> > + wl_keyboard_release(cl->input->keyboard->wl_keyboard);
> > + free(cl->input->keyboard);
> > + cl->input->keyboard = NULL;
> > +
> > + weston_test_device_release(cl->test->weston_test, "pointer");
> > + weston_test_device_release(cl->test->weston_test, "keyboard");
> > + client_roundtrip(cl);
> > +
> > + client_roundtrip(cl);
> > + assert(wl_display_get_error(cl->wl_display) == 0);
> > +
> > + /* restore previous state */
> > + weston_test_device_add(cl->test->weston_test, "pointer");
> > + weston_test_device_add(cl->test->weston_test, "keyboard");
> > + client_roundtrip(cl);
> > +}
> > +
> > +TEST(device_release_before_destroy_multiple)
> > +{
> > + int i;
> > +
> > + /* if weston crashed during this test, then there is
> > + * some inconsistency */
> > + for (i = 0; i < 100; ++i) {
> > + device_release_before_destroy();
> > + }
> > +}
> > +
> > +/* see https://bugzilla.gnome.org/show_bug.cgi?id=745008
> > + * It is a mutter bug, but highly relevant */
> > +TEST(get_device_after_destroy)
> > +{
> > + struct client *cl = client_create(100, 100, 100, 100);
> > + struct wl_pointer *wl_pointer;
> > + struct wl_keyboard *wl_keyboard;
> > +
> > + /* There's a race:
> > + * 1) compositor destroyes device
> > + * 2) client asks for the device, because it has not got
> > + * new capabilities yet
> > + * 3) compositor gets request with new_id for destroyed device
> > + * 4) client uses the new_id
> > + * 5) client gets new capabilities, destroying the objects
> > + *
> > + * If compositor just bail out in step 3) and does not create
> > + * resource, then client gets error in step 4) - even though
> > + * it followed the protocol (it just didn't know about new
> > + * capabilities).
> > + *
> > + * This test simulates this situation
> > + */
>
> A very good case to test indeed.
>
> > +
> > + /* connection is buffered, so after calling client_roundtrip(),
> > + * this whole batch will be delivered to compositor and will
> > + * exactly simulate our situation */
> > + weston_test_device_release(cl->test->weston_test, "pointer");
> > + wl_pointer = wl_seat_get_pointer(cl->input->wl_seat);
> > + assert(wl_pointer);
> > +
> > + /* this should be ignored */
> > + wl_pointer_set_cursor(wl_pointer, 0, NULL, 0, 0);
> > +
> > + /* this should not be ignored */
> > + wl_pointer_release(wl_pointer);
> > + client_roundtrip(cl);
> > +
> > + weston_test_device_release(cl->test->weston_test, "keyboard");
> > + wl_keyboard = wl_seat_get_keyboard(cl->input->wl_seat);
> > + assert(wl_keyboard);
> > + wl_keyboard_release(wl_keyboard);
> > + client_roundtrip(cl);
> > +
> > + client_roundtrip(cl);
> > + assert(wl_display_get_error(cl->wl_display) == 0);
> > +
> > + /* get weston to the previous state, so that other tests
> > + * have the same environment */
> > + weston_test_device_add(cl->test->weston_test, "pointer");
> > + weston_test_device_add(cl->test->weston_test, "keyboard");
> > + client_roundtrip(cl);
>
> Just for the future... maybe we should somehow make Weston restart for
> all or some tests? So clean-up like this would not be necessary. Or is
> that a bad idea?
>
It is not bad idea, but it needs to be configurable (so I'm in for: 'for
some tests').
Spawning Weston for all tests could bring big overhead and increase the
running time of tests significantly.
>
> > +}
> > +
> > +TEST(get_device_afer_destroy_multiple)
> > +{
> > + int i;
> > +
> > + /* if weston crashed during this test, then there is
> > + * some inconsistency */
> > + for (i = 0; i < 100; ++i) {
> > + get_device_after_destroy();
> > + }
> > +}
>
> I ran this series of 4 patches (one for wayland) and got:
>
> test-client: got global pointer 100 100
> test-client: got keyboard keymap
> test-client: got surface enter output 0x15d3720
> test-client: got keyboard modifiers 0 0 0 0
> test-client: got pointer enter 0 0, surface 0x15d3ad0
> libwayland: wl_display at 1: error 1: invalid method 1 (since 1 < 3), object
> wl_pointer at 13
> devices.weston: tests/devices-test.c:133: get_device_after_destroy:
> Assertion `wl_display_roundtrip((cl)->wl_display)
> >= 0' failed.
> test "get_device_afer_destroy_multiple": signal 6, fail.
>
> I suppose the client helpers do not bind the correct version. I think
> all tests that attempt to use release requests fail with this.
>
>
Oops, yes. I have patched it locally but totally forgot about it. I'll send
a patch.
>
> Is this the test that you expect Weston to fail?
> devices.weston: tests/devices-test.c:40: seat_capabilities_test: Assertion
> `!cl->input->pointer' failed.
>
Nope, not at all. Do you have more output? I can't reproduce it.
> There is also a fundamental weakness in all these tests. They assume
> that the seat has a keyboard and a pointer to begin with. That might
> not be the case when running with a real backend.
>
>
Yep. Actually, more (all?) tests assume that. For example keyboard-test.c
does not even check if the client has keyboard.
Maybe we should just skip the tests when the backend has insufficient
capabilities. (If we won't be using the test seat as you
proposed below)
> A obvious further addition would be the same for wl_touch.
>
Yes. wl_touch is not implemented at all in tests, so I did do so in this
test too. Once we implement it, we can add it.
>
> Maybe we should have a separate "test" seat, created by the test
> plugin? But I'm not sure how test clients would know to pick the right
> seat then.
>
>
Maybe we could just create test seat with missing objects. I mean when
backend provides seat with keyboard but nothing more,
we would create another seat that would provide pointer and touch. How is
it in such set-up? Can Weston handle multi-seat?
Or (almost) equivalently, the test plugin could just create the missing
devices on the default seat when they are not present.
>
> Thanks,
> pq
>
Thanks for review,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150323/18d74291/attachment-0001.html>
More information about the wayland-devel
mailing list