Re: [PATCH] tests/unit/test-char.c: Fix error handling issues

[Daniel P . Berrangé]

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.maydell@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.maydell@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/CAFEAcA9juOChqrh5orybJQwpQsyEZ5z3Dvmy7fjX0DW4Nbgmrg@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 <mclasen@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 :|