On Tue, Jun 08, 2021 at 11:51:35PM +0400, Marc-André Lureau wrote: > Hi > > On Tue, Jun 8, 2021 at 9:06 PM Peter Maydell <peter.mayd...@linaro.org> > wrote: > > > Coverity spots some minor error-handling issues in this test code. > > These are mostly due to the test code assuming that the glib test > > macros g_assert_cmpint() and friends will always abort on failure. > > This is not the case: if the test case chooses to call > > g_test_set_nonfatal_assertions() then they will mark the test case as > > a failure and continue. (This is different to g_assert(), > > g_assert_not_reached(), and assert(), which really do all always kill > > the process.) The idea is that you use g_assert() for things > > which are really assertions, as you would in normal QEMU code, > > and g_assert_cmpint() and friends for "this check is the thing > > this test case is testing" checks. > > > > In fact this test case does not currently set assertions to be > > nonfatal, but we should ideally be aiming to get to a point where we > > can set that more generally, because the test harness gives much > > better error reporting (including minor details like "what was the > > name of the test case that actually failed") than a raw assert/abort > > does. So we mostly fix the Coverity nits by making the error-exit > > path return early if necessary, rather than by converting the > > g_assert_cmpint()s to g_assert()s. > > > > Fixes: Coverity CID 1432505, 1432514, 1432600, 1451384 > > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> > > --- > > We had some previous not-very-conclusive discussion about > > g_assert_foo vs g_assert in this thread: > > > > https://lore.kernel.org/qemu-devel/cafeaca9juochqrh5orybjqwpqsyez5z3dvmy7fjx0dw4nbg...@mail.gmail.com/ > > This patch is in some sense me asserting my opinion about the > > right thing to do. You might disagree... > > > > I think that improving the quality of the failure reporting > > in 'make check' is useful, and that we should probably turn > > on g_test_set_nonfatal_assertions() everywhere. (The worst that > > can happen is that instead of crashing on the assert we proceed > > and crash a bit later, I think.) Awkwardly we don't have a single > > place where we could put that call, so I guess it's a coccinelle > > script to add it to every test's main() function. > > > > > I don't have any strong opinion on this. But I don't see much sense in > having extra code for things that should never happen. I would teach > coverity instead that those asserts are always fatal. aborting right where > the assert is reached is easier for the developer, as you get a direct > backtrace. Given that tests are usually grouped in domains, it doesn't help > much to keep running the rest of the tests in that group anyway. > > Fwiw, none of the tests in glib or gtk seem to use > g_test_set_nonfatal_assertions(), probably for similar considerations.
The method was introduced relatively recently (recent in glib terms, this was still 2013). commit a6a87506877939fee54bdc7eca70d47fc7d893d4 Author: Matthias Clasen <mcla...@redhat.com> Date: Sat Aug 17 15:18:29 2013 -0400 Add a way to make assertions non-fatal When using test harnesses other than gtester (e.g. using TAP), it can be suboptimal to have the very first failed assertion abort the test suite. This commit adds a g_test_set_nonfatal_assertions() that can be called in a test binary to change the behaviour of most assert macros to just call g_test_fail() and continue. We don't change the behavior of g_assert() and g_assert_not_reached(), since these to assertion macros are older than GTest, are widely used outside of testsuites, and will cause compiler warnings if they loose their noreturn annotation. https://bugzilla.gnome.org/show_bug.cgi?id=692125 This makes sense as a rationale so I'm surprised that they never then used it in glib tests. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|