| Message ID | 20240109192923.3183219-1-raj.khem@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] glib-networking: Fix ptest failures with openssl backend | expand |
On 9 Jan 2024, at 19:29, Khem Raj via lists.openembedded.org <raj.khem=gmail.com@lists.openembedded.org> wrote: > ++ #ifndef BACKEND_IS_OPENSSL > + g_test_add ("/tls/" BACKEND "/connection/unclean-close-by-server", TestConnection, NULL, > + setup_connection, test_unclean_close_by_server, teardown_connection); > ++ #endif Personally, unless upstream say “yes this test doesn’t work on openssl yet” and mark it as such, this is still a test failure that is failing for unknown reasons. We could #if 0 out every test that failed and rejoice in our 100% pass test cases but that would be a terrible idea, so why is this different? Ross
On Mon, 15 Jan 2024 at 14:41, Ross Burton <ross.burton@arm.com> wrote: > > On 9 Jan 2024, at 19:29, Khem Raj via lists.openembedded.org <raj.khem=gmail.com@lists.openembedded.org> wrote: > > ++ #ifndef BACKEND_IS_OPENSSL > > + g_test_add ("/tls/" BACKEND "/connection/unclean-close-by-server", TestConnection, NULL, > > + setup_connection, test_unclean_close_by_server, teardown_connection); > > ++ #endif > > Personally, unless upstream say “yes this test doesn’t work on openssl yet” and mark it as such, this is still a test failure that is failing for unknown reasons. > > We could #if 0 out every test that failed and rejoice in our 100% pass test cases but that would be a terrible idea, so why is this different? This is what we already do for intermittently failing ptests all over the stack. Rejoice! This issue is reported upstream, and the link is in the patch. Yes, given sufficient time one can become an expert in intricacies of glib, openssl, and their interaction, and then develop a real fix all by themselves, but I think a more fair expectation is exactly what Khem is doing: report upstream, add a reasonable workaround, follow up upstream requests in the reported ticket. Alex
On 15 Jan 2024, at 13:49, Alexander Kanavin <alex.kanavin@gmail.com> wrote: >> We could #if 0 out every test that failed and rejoice in our 100% pass test cases but that would be a terrible idea, so why is this different? > > This is what we already do for intermittently failing ptests all over > the stack. Rejoice! Hopefully we assess the test and determine if the odds are good that it is in fact timing related before doing this! > This issue is reported upstream, and the link is in the patch. Yes, > given sufficient time one can become an expert in intricacies of glib, > openssl, and their interaction, and then develop a real fix all by > themselves, but I think a more fair expectation is exactly what Khem > is doing: report upstream, add a reasonable workaround, follow up > upstream requests in the reported ticket. This is a configuration that upstream doesn’t support and we don’t enable by default, so I’m leaning towards Khem carries the patch alongside the bbappend to turn on the openssl backend in the first place. Ross
On Mon, 15 Jan 2024 at 15:04, Ross Burton <Ross.Burton@arm.com> wrote: > > This issue is reported upstream, and the link is in the patch. Yes, > > given sufficient time one can become an expert in intricacies of glib, > > openssl, and their interaction, and then develop a real fix all by > > themselves, but I think a more fair expectation is exactly what Khem > > is doing: report upstream, add a reasonable workaround, follow up > > upstream requests in the reported ticket. > > This is a configuration that upstream doesn’t support and we don’t enable by default, so I’m leaning towards Khem carries the patch alongside the bbappend to turn on the openssl backend in the first place. With this I agree. I don't think oe-core should carry patches for configurations it doesn't test. Alex
On Mon, Jan 15, 2024 at 5:41 AM Ross Burton <Ross.Burton@arm.com> wrote: > On 9 Jan 2024, at 19:29, Khem Raj via lists.openembedded.org <raj.khem= > gmail.com@lists.openembedded.org> wrote: > > ++ #ifndef BACKEND_IS_OPENSSL > > + g_test_add ("/tls/" BACKEND "/connection/unclean-close-by-server", > TestConnection, NULL, > > + setup_connection, test_unclean_close_by_server, > teardown_connection); > > ++ #endif > > Personally, unless upstream say “yes this test doesn’t work on openssl > yet” and mark it as such, this is still a test failure that is failing for > unknown reasons. > > We could #if 0 out every test that failed and rejoice in our 100% pass > test cases but that would be a terrible idea, so why is this different? > This however is not the case here. Upstream has accepted the bug already and marked it so as well. I am not sure why we should act more stricter in accepting patches than upstream activity here. > Ross
On Mon, Jan 15, 2024 at 6:04 AM Ross Burton <Ross.Burton@arm.com> wrote: > On 15 Jan 2024, at 13:49, Alexander Kanavin <alex.kanavin@gmail.com> > wrote: > >> We could #if 0 out every test that failed and rejoice in our 100% pass > test cases but that would be a terrible idea, so why is this different? > > > > This is what we already do for intermittently failing ptests all over > > the stack. Rejoice! > > Hopefully we assess the test and determine if the odds are good that it is > in fact timing related before doing this! > > > This issue is reported upstream, and the link is in the patch. Yes, > > given sufficient time one can become an expert in intricacies of glib, > > openssl, and their interaction, and then develop a real fix all by > > themselves, but I think a more fair expectation is exactly what Khem > > is doing: report upstream, add a reasonable workaround, follow up > > upstream requests in the reported ticket. > > This is a configuration that upstream doesn’t support and we don’t enable > by default, so I’m leaning towards Khem carries the patch alongside the > bbappend to turn on the openssl backend in the first place. Where does it say it does not support? On the contrary there are patches in this area sent recently Yes I can carry all of changes we need locally that’s not an issue we are already carrying many of them they just make it harder to contribute back to project as these pile up > > Ross
> On 15 Jan 2024, at 16:53, Khem Raj <raj.khem@gmail.com> wrote: > Where does it say it does not support? On the contrary there are patches in this area sent recently https://gitlab.gnome.org/GNOME/glib-networking/-/commit/8e1d80c1e0fc52d17d08a21946fa4a86ec30e1db Ross
On Mon, Jan 15, 2024 at 9:06 AM Ross Burton <Ross.Burton@arm.com> wrote: > > > > > On 15 Jan 2024, at 16:53, Khem Raj <raj.khem@gmail.com> wrote: > > Where does it say it does not support? On the contrary there are patches in this area sent recently > > https://gitlab.gnome.org/GNOME/glib-networking/-/commit/8e1d80c1e0fc52d17d08a21946fa4a86ec30e1db > Right, it was 3 years ago and note the mention kind of systems where it maybe in use :) and OE is used for such distros so it might be useful. The fixes to get openssl support improved have happened after this commit. Its fine if we do not want to accept patches here and wait upstream to support it fully. Because than it's easier to direct upstream work correctly. We just have to be clear with these criterias in general. > Ross
On Mon, Jan 15, 2024 at 11:38 AM Khem Raj <raj.khem@gmail.com> wrote: > > On Mon, Jan 15, 2024 at 9:06 AM Ross Burton <Ross.Burton@arm.com> wrote: > > > > > > > > > On 15 Jan 2024, at 16:53, Khem Raj <raj.khem@gmail.com> wrote: > > > Where does it say it does not support? On the contrary there are patches in this area sent recently > > > > https://gitlab.gnome.org/GNOME/glib-networking/-/commit/8e1d80c1e0fc52d17d08a21946fa4a86ec30e1db > > > > Right, it was 3 years ago and note the mention kind of systems where > it maybe in use :) and OE is used for such > distros so it might be useful. The fixes to get openssl support > improved have happened after this commit. > Its fine if we do not want to accept patches here and wait upstream to > support it fully. Because than it's easier to direct > upstream work correctly. We just have to be clear with these criterias > in general. Lets drop this patch. > > > Ross
diff --git a/meta/recipes-core/glib-networking/glib-networking/0001-tls-tests-connection.c-Disable-unclean-close-by-serv.patch b/meta/recipes-core/glib-networking/glib-networking/0001-tls-tests-connection.c-Disable-unclean-close-by-serv.patch new file mode 100644 index 00000000000..be7fc586f40 --- /dev/null +++ b/meta/recipes-core/glib-networking/glib-networking/0001-tls-tests-connection.c-Disable-unclean-close-by-serv.patch @@ -0,0 +1,36 @@ +From fdcd8fbf23983d33327967f3e4249bba7c5a1b93 Mon Sep 17 00:00:00 2001 +From: Khem Raj <raj.khem@gmail.com> +Date: Tue, 9 Jan 2024 11:15:21 -0800 +Subject: [PATCH] tls/tests/connection.c: Disable unclean-close-by-server with + openssl backend + +This test does not work when using openssl backend as reported here [1] + +Fixes + +not ok /tls/openssl/connection/unclean-close-by-server - GLib-Net:ERROR:../tls/tests/connection.c:2374:test_unclean_close_by_server: assertion failed (test->read_error == (g-tls-error-quark, 6)): Error reading data from TLS socket: error:00000005:lib(0)::reason(5) (g-tls-error-quark, 1) Bail out! +stderr: +** +GLib-Net:ERROR:../tls/tests/connection.c:2374:test_unclean_close_by_server: assertion failed (test->read_error == (g-tls-error-quark, 6)): Error reading data from TLS socket: error:00000005:lib(0)::reason(5) (g-tls-error-quark, 1) + +[1] https://gitlab.gnome.org/GNOME/glib-networking/-/issues/219 + +Upstream-Status: Inappropriate [ Disabled test as a workaround until fixed ] +Signed-off-by: Khem Raj <raj.khem@gmail.com> +--- + tls/tests/connection.c | 2 ++ + 1 file changed, 2 insertions(+) + +--- a/tls/tests/connection.c ++++ b/tls/tests/connection.c +@@ -3466,8 +3466,10 @@ main (int argc, + setup_connection, test_simultaneous_sync_rehandshake, teardown_connection); + g_test_add ("/tls/" BACKEND "/connection/close-immediately", TestConnection, NULL, + setup_connection, test_close_immediately, teardown_connection); ++ #ifndef BACKEND_IS_OPENSSL + g_test_add ("/tls/" BACKEND "/connection/unclean-close-by-server", TestConnection, NULL, + setup_connection, test_unclean_close_by_server, teardown_connection); ++ #endif + g_test_add ("/tls/" BACKEND "/connection/async-implicit-handshake", TestConnection, NULL, + setup_connection, test_async_implicit_handshake, teardown_connection); + g_test_add ("/tls/" BACKEND "/connection/output-stream-close", TestConnection, NULL, diff --git a/meta/recipes-core/glib-networking/glib-networking_2.78.0.bb b/meta/recipes-core/glib-networking/glib-networking_2.78.0.bb index 68f9a2ed783..3e83bb6ca3b 100644 --- a/meta/recipes-core/glib-networking/glib-networking_2.78.0.bb +++ b/meta/recipes-core/glib-networking/glib-networking_2.78.0.bb @@ -30,8 +30,9 @@ PACKAGECONFIG[gnomeproxy] = "-Dgnome_proxy=enabled,-Dgnome_proxy=disabled,gsetti inherit gnomebase gettext upstream-version-is-even gio-module-cache ptest-gnome SRC_URI += "file://run-ptest" -SRC_URI += "file://eagain.patch" - +SRC_URI += "file://eagain.patch \ + file://0001-tls-tests-connection.c-Disable-unclean-close-by-serv.patch \ + " FILES:${PN} += "\ ${libdir}/gio/modules/libgio*.so \ ${datadir}/dbus-1/services/ \
Signed-off-by: Khem Raj <raj.khem@gmail.com> --- v2: Make the patch apply only when openssl backend is used v3: Move the backend check to source code of the test itself ...tion.c-Disable-unclean-close-by-serv.patch | 36 +++++++++++++++++++ .../glib-networking/glib-networking_2.78.0.bb | 5 +-- 2 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 meta/recipes-core/glib-networking/glib-networking/0001-tls-tests-connection.c-Disable-unclean-close-by-serv.patch