Re: [PATCH 1/1] tests: Add free-without-remove test
On 2018/2月/23 08:53, Derek Foreman wrote:
> On 2018-02-23 07:44 AM, Markus Ongyerth wrote:
> > On 2018/2月/23 07:31, Derek Foreman wrote:
> > > On 2018-02-23 03:52 AM, w...@ongy.net wrote:
> > > > From: Markus Ongyerth
> > > >
> > > > This is related to the discussion earlier on the mailing list and
> > > > demonstrates a problem with current wl_priv_signal_emit and wl_listener
> > > > that free themselves, but do not remove from the list.
> > > >
> > > > This testcase asserts that the wl_list inside wl_listener is not written
> > > > to anymore after it got emitted.
> > > > ---
> > > >Makefile.am | 3 +++
> > > >tests/free-without-remove.c | 63
> > > > +
> > > >2 files changed, 66 insertions(+)
> > > >create mode 100644 tests/free-without-remove.c
> > > >
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 322d6b8..c51f328 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
> > > >built_test_programs =\
> > > > array-test \
> > > > + free-without-remove-test\
> > > > client-test \
> > > > display-test\
> > > > connection-test \
> > > > @@ -223,6 +224,8 @@ libtest_runner_la_LIBADD = \
> > > >array_test_SOURCES = tests/array-test.c
> > > >array_test_LDADD = libtest-runner.la
> > > > +free_without_remove_test_SOURCES = tests/free-without-remove.c
> > > > +free_without_remove_test_LDADD = libtest-runner.la
> > > >client_test_SOURCES = tests/client-test.c
> > > >client_test_LDADD = libtest-runner.la
> > > >display_test_SOURCES = tests/display-test.c
> > > > diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
> > > > new file mode 100644
> > > > index 000..ff468d7
> > > > --- /dev/null
> > > > +++ b/tests/free-without-remove.c
> > > > @@ -0,0 +1,63 @@
> > > > +/*
> > > > + * Copyright © 2018 Markus Ongyerth
> > > > + *
> > > > + * Permission is hereby granted, free of charge, to any person
> > > > obtaining
> > > > + * a copy of this software and associated documentation files (the
> > > > + * "Software"), to deal in the Software without restriction, including
> > > > + * without limitation the rights to use, copy, modify, merge, publish,
> > > > + * distribute, sublicense, and/or sell copies of the Software, and to
> > > > + * permit persons to whom the Software is furnished to do so, subject
> > > > to
> > > > + * the following conditions:
> > > > + *
> > > > + * The above copyright notice and this permission notice (including the
> > > > + * next paragraph) shall be included in all copies or substantial
> > > > + * portions of the Software.
> > > > + *
> > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > > > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> > > > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> > > > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> > > > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> > > > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > > > + * SOFTWARE.
> > > > + */
> > > > +
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +
> > > > +#include "wayland-private.h"
> > > > +#include "wayland-server.h"
> > > > +#include "test-runner.h"
> > > > +
> > > > +static void
> > > > +display_destroy_notify(struct wl_listener *l, void *data)
> > > > +{
> > > > + l->link.prev = l->link.next = NULL;
> > > > +}
> > > > +
> > > > +TEST(free_without_remove)
> > > > +{
> > > > + struct wl_display *display;
> > > > + struct wl_listener a, b;
> > > > +
> > > > + display = wl_display_create();
> > > > + a.notify = display_destroy_notify;
> > > > + b.notify = display_destroy_notify;
> > > > +
> > > > + wl_display_add_destroy_listener(display, &a);
> > > > + wl_display_add_destroy_listener(display, &b);
> > > > +
> > > > + wl_display_destroy(display);
> > > > +
> > > > + assert(a.link.next == a.link.prev && a.link.next == NULL);
> > > > + assert(b.link.next == b.link.prev && b.link.next == NULL);
> > >
> > > I feel this would be slightly more clear if it tested
> > > a.link.next == NULL && a.link.prev == NULL
> > Possibly.
> > >
> > > As my previously submit test did... is this significantly more rigorous
> > > than
> > > my test here: https://patchwork.freedesktop.org/patch/206013/ ?
> > Did you try to run this one? It *fails* on the current versio
Re: [PATCH 1/1] tests: Add free-without-remove test
On 2018-02-23 07:44 AM, Markus Ongyerth wrote:
On 2018/2月/23 07:31, Derek Foreman wrote:
On 2018-02-23 03:52 AM, w...@ongy.net wrote:
From: Markus Ongyerth
This is related to the discussion earlier on the mailing list and
demonstrates a problem with current wl_priv_signal_emit and wl_listener
that free themselves, but do not remove from the list.
This testcase asserts that the wl_list inside wl_listener is not written
to anymore after it got emitted.
---
Makefile.am | 3 +++
tests/free-without-remove.c | 63
+
2 files changed, 66 insertions(+)
create mode 100644 tests/free-without-remove.c
diff --git a/Makefile.am b/Makefile.am
index 322d6b8..c51f328 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
built_test_programs =\
array-test \
+ free-without-remove-test\
client-test \
display-test\
connection-test \
@@ -223,6 +224,8 @@ libtest_runner_la_LIBADD = \
array_test_SOURCES = tests/array-test.c
array_test_LDADD = libtest-runner.la
+free_without_remove_test_SOURCES = tests/free-without-remove.c
+free_without_remove_test_LDADD = libtest-runner.la
client_test_SOURCES = tests/client-test.c
client_test_LDADD = libtest-runner.la
display_test_SOURCES = tests/display-test.c
diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
new file mode 100644
index 000..ff468d7
--- /dev/null
+++ b/tests/free-without-remove.c
@@ -0,0 +1,63 @@
+/*
+ * Copyright © 2018 Markus Ongyerth
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "wayland-private.h"
+#include "wayland-server.h"
+#include "test-runner.h"
+
+static void
+display_destroy_notify(struct wl_listener *l, void *data)
+{
+ l->link.prev = l->link.next = NULL;
+}
+
+TEST(free_without_remove)
+{
+ struct wl_display *display;
+ struct wl_listener a, b;
+
+ display = wl_display_create();
+ a.notify = display_destroy_notify;
+ b.notify = display_destroy_notify;
+
+ wl_display_add_destroy_listener(display, &a);
+ wl_display_add_destroy_listener(display, &b);
+
+ wl_display_destroy(display);
+
+ assert(a.link.next == a.link.prev && a.link.next == NULL);
+ assert(b.link.next == b.link.prev && b.link.next == NULL);
I feel this would be slightly more clear if it tested
a.link.next == NULL && a.link.prev == NULL
Possibly.
As my previously submit test did... is this significantly more rigorous than
my test here: https://patchwork.freedesktop.org/patch/206013/ ?
Did you try to run this one? It *fails* on the current version of libwayland,
because wl_priv_signal_emit will write into one of the wl_listeners after it
got emitted.
This only happens where there's more than one listener in the signal list,
which is the reason your test doesn't catch it.
More rigorous indeed - but we can't land it without fixing the bug first. :)
Also, I'm not sure it warrants a whole new file when it could just piggy
back an existing test... I guess now that I think about it, signal-test
seems most appropriate?
Thanks,
Derek
Thanks,
Derek
+}
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 1/1] tests: Add free-without-remove test
On 2018/2月/23 07:31, Derek Foreman wrote:
> On 2018-02-23 03:52 AM, w...@ongy.net wrote:
> > From: Markus Ongyerth
> >
> > This is related to the discussion earlier on the mailing list and
> > demonstrates a problem with current wl_priv_signal_emit and wl_listener
> > that free themselves, but do not remove from the list.
> >
> > This testcase asserts that the wl_list inside wl_listener is not written
> > to anymore after it got emitted.
> > ---
> > Makefile.am | 3 +++
> > tests/free-without-remove.c | 63
> > +
> > 2 files changed, 66 insertions(+)
> > create mode 100644 tests/free-without-remove.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 322d6b8..c51f328 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
> > built_test_programs = \
> > array-test \
> > + free-without-remove-test\
> > client-test \
> > display-test\
> > connection-test \
> > @@ -223,6 +224,8 @@ libtest_runner_la_LIBADD = \
> > array_test_SOURCES = tests/array-test.c
> > array_test_LDADD = libtest-runner.la
> > +free_without_remove_test_SOURCES = tests/free-without-remove.c
> > +free_without_remove_test_LDADD = libtest-runner.la
> > client_test_SOURCES = tests/client-test.c
> > client_test_LDADD = libtest-runner.la
> > display_test_SOURCES = tests/display-test.c
> > diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
> > new file mode 100644
> > index 000..ff468d7
> > --- /dev/null
> > +++ b/tests/free-without-remove.c
> > @@ -0,0 +1,63 @@
> > +/*
> > + * Copyright © 2018 Markus Ongyerth
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining
> > + * a copy of this software and associated documentation files (the
> > + * "Software"), to deal in the Software without restriction, including
> > + * without limitation the rights to use, copy, modify, merge, publish,
> > + * distribute, sublicense, and/or sell copies of the Software, and to
> > + * permit persons to whom the Software is furnished to do so, subject to
> > + * the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > + * next paragraph) shall be included in all copies or substantial
> > + * portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > + * SOFTWARE.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "wayland-private.h"
> > +#include "wayland-server.h"
> > +#include "test-runner.h"
> > +
> > +static void
> > +display_destroy_notify(struct wl_listener *l, void *data)
> > +{
> > + l->link.prev = l->link.next = NULL;
> > +}
> > +
> > +TEST(free_without_remove)
> > +{
> > + struct wl_display *display;
> > + struct wl_listener a, b;
> > +
> > + display = wl_display_create();
> > + a.notify = display_destroy_notify;
> > + b.notify = display_destroy_notify;
> > +
> > + wl_display_add_destroy_listener(display, &a);
> > + wl_display_add_destroy_listener(display, &b);
> > +
> > + wl_display_destroy(display);
> > +
> > + assert(a.link.next == a.link.prev && a.link.next == NULL);
> > + assert(b.link.next == b.link.prev && b.link.next == NULL);
>
> I feel this would be slightly more clear if it tested
> a.link.next == NULL && a.link.prev == NULL
Possibly.
>
> As my previously submit test did... is this significantly more rigorous than
> my test here: https://patchwork.freedesktop.org/patch/206013/ ?
Did you try to run this one? It *fails* on the current version of libwayland,
because wl_priv_signal_emit will write into one of the wl_listeners after it
got emitted.
This only happens where there's more than one listener in the signal list,
which is the reason your test doesn't catch it.
>
> Thanks,
> Derek
>
> > +}
> >
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/
Re: [PATCH 1/1] tests: Add free-without-remove test
On 2018-02-23 03:52 AM, w...@ongy.net wrote:
From: Markus Ongyerth
This is related to the discussion earlier on the mailing list and
demonstrates a problem with current wl_priv_signal_emit and wl_listener
that free themselves, but do not remove from the list.
This testcase asserts that the wl_list inside wl_listener is not written
to anymore after it got emitted.
---
Makefile.am | 3 +++
tests/free-without-remove.c | 63 +
2 files changed, 66 insertions(+)
create mode 100644 tests/free-without-remove.c
diff --git a/Makefile.am b/Makefile.am
index 322d6b8..c51f328 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
built_test_programs =\
array-test \
+ free-without-remove-test\
client-test \
display-test\
connection-test \
@@ -223,6 +224,8 @@ libtest_runner_la_LIBADD = \
array_test_SOURCES = tests/array-test.c
array_test_LDADD = libtest-runner.la
+free_without_remove_test_SOURCES = tests/free-without-remove.c
+free_without_remove_test_LDADD = libtest-runner.la
client_test_SOURCES = tests/client-test.c
client_test_LDADD = libtest-runner.la
display_test_SOURCES = tests/display-test.c
diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
new file mode 100644
index 000..ff468d7
--- /dev/null
+++ b/tests/free-without-remove.c
@@ -0,0 +1,63 @@
+/*
+ * Copyright © 2018 Markus Ongyerth
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "wayland-private.h"
+#include "wayland-server.h"
+#include "test-runner.h"
+
+static void
+display_destroy_notify(struct wl_listener *l, void *data)
+{
+ l->link.prev = l->link.next = NULL;
+}
+
+TEST(free_without_remove)
+{
+ struct wl_display *display;
+ struct wl_listener a, b;
+
+ display = wl_display_create();
+ a.notify = display_destroy_notify;
+ b.notify = display_destroy_notify;
+
+ wl_display_add_destroy_listener(display, &a);
+ wl_display_add_destroy_listener(display, &b);
+
+ wl_display_destroy(display);
+
+ assert(a.link.next == a.link.prev && a.link.next == NULL);
+ assert(b.link.next == b.link.prev && b.link.next == NULL);
I feel this would be slightly more clear if it tested
a.link.next == NULL && a.link.prev == NULL
As my previously submit test did... is this significantly more rigorous
than my test here: https://patchwork.freedesktop.org/patch/206013/ ?
Thanks,
Derek
+}
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
