Re: [PATCH wayland 1/2] tests: add test for callback serials

2014-10-31 Thread Marek Chalupa
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

2014-10-29 Thread Bryce Harrington
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