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

Submitted by Marek Chalupa on Oct. 27, 2014, 8:14 a.m.

Details

Message ID 1414397681-10678-1-git-send-email-mchqwerty@gmail.com
State Changes Requested
Headers show

Not browsing as part of any series.

Commit Message

Marek Chalupa Oct. 27, 2014, 8:14 a.m.
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 <mchqwerty@gmail.com>
---
 tests/display-test.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Patch hide | download patch | download mbox

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);
+}

Comments

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 <mchqwerty@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.harrington@samsung.com>

Bryce
On 29 October 2014 20:26, Bryce Harrington <bryce@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 <mchqwerty@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.harrington@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