Re: [PATCH wayland 1/2] tests: add test for callback serials
On 29 October 2014 20:26, Bryce Harrington br...@osg.samsung.com wrote: On Mon, Oct 27, 2014 at 09:14:40AM +0100, Marek Chalupa wrote: The callback returns always with the same serial, which is not right (it's serial, not constant...). This test highlights the bug. Signed-off-by: Marek Chalupa mchqwe...@gmail.com --- tests/display-test.c | 55 1 file changed, 55 insertions(+) diff --git a/tests/display-test.c b/tests/display-test.c index a1e45b1..eb4ba1c 100644 --- a/tests/display-test.c +++ b/tests/display-test.c @@ -593,3 +593,58 @@ TEST(threading_read_after_error_tst) display_destroy(d); } + +static void +sync_callback(void *data, struct wl_callback *wl_callback, uint32_t serial) +{ + int *got = data; + static uint32_t last_serial = 0; + + /* if this is the first callback, just copy the value of serial */ + if (*got == 0) + last_serial = serial; + else + ++last_serial; + + /* since we only call display_sync, nothing else can increase the + * serial, so the serails must be sequential */ sp. serials + assert(serial == last_serial + Serial is not sequential); + + ++(*got); I'm a bit confused about the intention of 'got', is it just a counter for how many times the callback is called? Would sync_callback_count be a clearer variable name? Yes, it is just a counter. I could have chosen better name -- like the one you propose, I'll fix it. +} + +static const struct wl_callback_listener sync_listener = { + sync_callback +}; + +#define CB_NUM 1000 +static void +callback_serial_tst_main(void) +{ + int i, got = 0; + struct wl_callback *cb; + struct client *client = client_connect(); 'client' maybe not a good choice of variable name since it's also the type name. 'c' seems to be convention here, so maybe: It's used in other tests too, but I don't really mind using 'c' or 'cli' or something else instead of client, so can fix it. struct client *c = client_connect(); + for (i = 0; i CB_NUM; ++i) { + cb = wl_display_sync(client-wl_display); + wl_callback_add_listener(cb, sync_listener, got); + } + + wl_display_flush(client-wl_display); + wl_display_roundtrip(client-wl_display); + + assert(got == CB_NUM Lost some callback); + + client_disconnect(client); +} + +TEST(callback_serial_tst) +{ + struct display *d = display_create(); + + client_create(d, callback_serial_tst_main); + display_run(d); + + display_destroy(d); +} -- 1.9.3 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel Other than the few quibbles, looks quite good. And cheers for adding a test with the bugfix. :-) Reviewed-by: Bryce Harrington b.harring...@samsung.com Bryce Thanks for reviewing! Few days ago I found out that on bugzilla is a bug regarding the serials. https://bugs.freedesktop.org/show_bug.cgi?id=83488 I added a comment there and I'll wait with the changes to this patch until I get some feedback (it's quite possible that this patch won't be push at all). Thanks, Marek ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 1/2] tests: add test for callback serials
On Mon, Oct 27, 2014 at 09:14:40AM +0100, Marek Chalupa wrote: The callback returns always with the same serial, which is not right (it's serial, not constant...). This test highlights the bug. Signed-off-by: Marek Chalupa mchqwe...@gmail.com --- tests/display-test.c | 55 1 file changed, 55 insertions(+) diff --git a/tests/display-test.c b/tests/display-test.c index a1e45b1..eb4ba1c 100644 --- a/tests/display-test.c +++ b/tests/display-test.c @@ -593,3 +593,58 @@ TEST(threading_read_after_error_tst) display_destroy(d); } + +static void +sync_callback(void *data, struct wl_callback *wl_callback, uint32_t serial) +{ + int *got = data; + static uint32_t last_serial = 0; + + /* if this is the first callback, just copy the value of serial */ + if (*got == 0) + last_serial = serial; + else + ++last_serial; + + /* since we only call display_sync, nothing else can increase the + * serial, so the serails must be sequential */ sp. serials + assert(serial == last_serial + Serial is not sequential); + + ++(*got); I'm a bit confused about the intention of 'got', is it just a counter for how many times the callback is called? Would sync_callback_count be a clearer variable name? +} + +static const struct wl_callback_listener sync_listener = { + sync_callback +}; + +#define CB_NUM 1000 +static void +callback_serial_tst_main(void) +{ + int i, got = 0; + struct wl_callback *cb; + struct client *client = client_connect(); 'client' maybe not a good choice of variable name since it's also the type name. 'c' seems to be convention here, so maybe: struct client *c = client_connect(); + for (i = 0; i CB_NUM; ++i) { + cb = wl_display_sync(client-wl_display); + wl_callback_add_listener(cb, sync_listener, got); + } + + wl_display_flush(client-wl_display); + wl_display_roundtrip(client-wl_display); + + assert(got == CB_NUM Lost some callback); + + client_disconnect(client); +} + +TEST(callback_serial_tst) +{ + struct display *d = display_create(); + + client_create(d, callback_serial_tst_main); + display_run(d); + + display_destroy(d); +} -- 1.9.3 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel Other than the few quibbles, looks quite good. And cheers for adding a test with the bugfix. :-) Reviewed-by: Bryce Harrington b.harring...@samsung.com Bryce ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH wayland 1/2] tests: add test for callback serials
The callback returns always with the same serial, which is not right (it's serial, not constant...). This test highlights the bug. Signed-off-by: Marek Chalupa mchqwe...@gmail.com --- tests/display-test.c | 55 1 file changed, 55 insertions(+) diff --git a/tests/display-test.c b/tests/display-test.c index a1e45b1..eb4ba1c 100644 --- a/tests/display-test.c +++ b/tests/display-test.c @@ -593,3 +593,58 @@ TEST(threading_read_after_error_tst) display_destroy(d); } + +static void +sync_callback(void *data, struct wl_callback *wl_callback, uint32_t serial) +{ + int *got = data; + static uint32_t last_serial = 0; + + /* if this is the first callback, just copy the value of serial */ + if (*got == 0) + last_serial = serial; + else + ++last_serial; + + /* since we only call display_sync, nothing else can increase the +* serial, so the serails must be sequential */ + assert(serial == last_serial + Serial is not sequential); + + ++(*got); +} + +static const struct wl_callback_listener sync_listener = { + sync_callback +}; + +#define CB_NUM 1000 +static void +callback_serial_tst_main(void) +{ + int i, got = 0; + struct wl_callback *cb; + struct client *client = client_connect(); + + for (i = 0; i CB_NUM; ++i) { + cb = wl_display_sync(client-wl_display); + wl_callback_add_listener(cb, sync_listener, got); + } + + wl_display_flush(client-wl_display); + wl_display_roundtrip(client-wl_display); + + assert(got == CB_NUM Lost some callback); + + client_disconnect(client); +} + +TEST(callback_serial_tst) +{ + struct display *d = display_create(); + + client_create(d, callback_serial_tst_main); + display_run(d); + + display_destroy(d); +} -- 1.9.3 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel