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