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.

 tests/unit/test-char.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
index 5b3b48ebacd..43630ab57f8 100644
--- a/tests/unit/test-char.c
+++ b/tests/unit/test-char.c
@@ -214,6 +214,10 @@ static void char_mux_test(void)
     qemu_chr_fe_take_focus(&chr_be2);
 
     base = qemu_chr_find("mux-label-base");
+    g_assert_nonnull(base);
+    if (base == 0) {
+        goto fail;
+    }
     g_assert_cmpint(qemu_chr_be_can_write(base), !=, 0);
 
     qemu_chr_be_write(base, (void *)"hello", 6);
@@ -333,6 +337,7 @@ static void char_mux_test(void)
     g_assert_cmpint(strlen(data), !=, 0);
     g_free(data);
 
+fail:
     qemu_chr_fe_deinit(&chr_be1, false);
     qemu_chr_fe_deinit(&chr_be2, true);
 }
@@ -486,6 +491,9 @@ static void char_pipe_test(void)
     chr = qemu_chr_new("pipe", tmp, NULL);
     g_assert_nonnull(chr);
     g_free(tmp);
+    if (!chr) {
+        goto fail;
+    }
 
     qemu_chr_fe_init(&be, chr, &error_abort);
 
@@ -493,12 +501,20 @@ static void char_pipe_test(void)
     g_assert_cmpint(ret, ==, 9);
 
     fd = open(out, O_RDWR);
+    g_assert_cmpint(fd, >=, 0);
+    if (fd < 0) {
+        goto fail;
+    }
     ret = read(fd, buf, sizeof(buf));
     g_assert_cmpint(ret, ==, 9);
     g_assert_cmpstr(buf, ==, "pipe-out");
     close(fd);
 
     fd = open(in, O_WRONLY);
+    g_assert_cmpint(fd, >=, 0);
+    if (fd < 0) {
+        goto fail;
+    }
     ret = write(fd, "pipe-in", 8);
     g_assert_cmpint(ret, ==, 8);
     close(fd);
@@ -518,6 +534,7 @@ static void char_pipe_test(void)
 
     qemu_chr_fe_deinit(&be, true);
 
+fail:
     g_assert(g_unlink(in) == 0);
     g_assert(g_unlink(out) == 0);
     g_assert(g_rmdir(tmp_path) == 0);
@@ -556,7 +573,10 @@ static int make_udp_socket(int *port)
     socklen_t alen = sizeof(addr);
     int ret, sock = qemu_socket(PF_INET, SOCK_DGRAM, 0);
 
-    g_assert_cmpint(sock, >, 0);
+    g_assert_cmpint(sock, >=, 0);
+    if (sock < 0) {
+        return sock;
+    }
     addr.sin_family = AF_INET ;
     addr.sin_addr.s_addr = htonl(INADDR_ANY);
     addr.sin_port = 0;
@@ -586,6 +606,9 @@ static void char_udp_test_internal(Chardev *reuse_chr, int sock)
     } else {
         int port;
         sock = make_udp_socket(&port);
+        if (sock < 0) {
+            return;
+        }
         tmp = g_strdup_printf("udp:127.0.0.1:%d", port);
         chr = qemu_chr_new("client", tmp, NULL);
         g_assert_nonnull(chr);
@@ -1224,6 +1247,10 @@ static void char_file_fifo_test(void)
     }
 
     fd = open(fifo, O_RDWR);
+    g_assert_cmpint(fd, >=, 0);
+    if (fd < 0) {
+        goto fail;
+    }
     ret = write(fd, "fifo-in", 8);
     g_assert_cmpint(ret, ==, 8);
 
@@ -1253,6 +1280,7 @@ static void char_file_fifo_test(void)
 
     qemu_chr_fe_deinit(&be, true);
 
+fail:
     g_unlink(fifo);
     g_free(fifo);
     g_unlink(out);
@@ -1371,7 +1399,7 @@ static int chardev_change_denied(void *opaque)
 
 static void char_hotswap_test(void)
 {
-    char *chr_args;
+    char *chr_args = NULL;
     Chardev *chr;
     CharBackend be;
 
@@ -1385,6 +1413,9 @@ static void char_hotswap_test(void)
     int port;
     int sock = make_udp_socket(&port);
     g_assert_cmpint(sock, >, 0);
+    if (sock < 0) {
+        goto fail;
+    }
 
     chr_args = g_strdup_printf("udp:127.0.0.1:%d", port);
 
@@ -1422,6 +1453,7 @@ static void char_hotswap_test(void)
     object_unparent(OBJECT(chr));
 
     qapi_free_ChardevReturn(ret);
+fail:
     g_unlink(filename);
     g_free(filename);
     g_rmdir(tmp_path);
-- 
2.20.1

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.

 tests/unit/test-char.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
> index 5b3b48ebacd..43630ab57f8 100644
> --- a/tests/unit/test-char.c
> +++ b/tests/unit/test-char.c
> @@ -214,6 +214,10 @@ static void char_mux_test(void)
>      qemu_chr_fe_take_focus(&chr_be2);
>
>      base = qemu_chr_find("mux-label-base");
> +    g_assert_nonnull(base);
> +    if (base == 0) {
> +        goto fail;
> +    }
>      g_assert_cmpint(qemu_chr_be_can_write(base), !=, 0);
>
>      qemu_chr_be_write(base, (void *)"hello", 6);
> @@ -333,6 +337,7 @@ static void char_mux_test(void)
>      g_assert_cmpint(strlen(data), !=, 0);
>      g_free(data);
>
> +fail:
>      qemu_chr_fe_deinit(&chr_be1, false);
>      qemu_chr_fe_deinit(&chr_be2, true);
>  }
> @@ -486,6 +491,9 @@ static void char_pipe_test(void)
>      chr = qemu_chr_new("pipe", tmp, NULL);
>      g_assert_nonnull(chr);
>      g_free(tmp);
> +    if (!chr) {
> +        goto fail;
> +    }
>
>      qemu_chr_fe_init(&be, chr, &error_abort);
>
> @@ -493,12 +501,20 @@ static void char_pipe_test(void)
>      g_assert_cmpint(ret, ==, 9);
>
>      fd = open(out, O_RDWR);
> +    g_assert_cmpint(fd, >=, 0);
> +    if (fd < 0) {
> +        goto fail;
> +    }
>      ret = read(fd, buf, sizeof(buf));
>      g_assert_cmpint(ret, ==, 9);
>      g_assert_cmpstr(buf, ==, "pipe-out");
>      close(fd);
>
>      fd = open(in, O_WRONLY);
> +    g_assert_cmpint(fd, >=, 0);
> +    if (fd < 0) {
> +        goto fail;
> +    }
>      ret = write(fd, "pipe-in", 8);
>      g_assert_cmpint(ret, ==, 8);
>      close(fd);
> @@ -518,6 +534,7 @@ static void char_pipe_test(void)
>
>      qemu_chr_fe_deinit(&be, true);
>
> +fail:
>      g_assert(g_unlink(in) == 0);
>      g_assert(g_unlink(out) == 0);
>      g_assert(g_rmdir(tmp_path) == 0);
> @@ -556,7 +573,10 @@ static int make_udp_socket(int *port)
>      socklen_t alen = sizeof(addr);
>      int ret, sock = qemu_socket(PF_INET, SOCK_DGRAM, 0);
>
> -    g_assert_cmpint(sock, >, 0);
> +    g_assert_cmpint(sock, >=, 0);
> +    if (sock < 0) {
> +        return sock;
> +    }
>      addr.sin_family = AF_INET ;
>      addr.sin_addr.s_addr = htonl(INADDR_ANY);
>      addr.sin_port = 0;
> @@ -586,6 +606,9 @@ static void char_udp_test_internal(Chardev *reuse_chr,
> int sock)
>      } else {
>          int port;
>          sock = make_udp_socket(&port);
> +        if (sock < 0) {
> +            return;
> +        }
>          tmp = g_strdup_printf("udp:127.0.0.1:%d", port);
>          chr = qemu_chr_new("client", tmp, NULL);
>          g_assert_nonnull(chr);
> @@ -1224,6 +1247,10 @@ static void char_file_fifo_test(void)
>      }
>
>      fd = open(fifo, O_RDWR);
> +    g_assert_cmpint(fd, >=, 0);
> +    if (fd < 0) {
> +        goto fail;
> +    }
>      ret = write(fd, "fifo-in", 8);
>      g_assert_cmpint(ret, ==, 8);
>
> @@ -1253,6 +1280,7 @@ static void char_file_fifo_test(void)
>
>      qemu_chr_fe_deinit(&be, true);
>
> +fail:
>      g_unlink(fifo);
>      g_free(fifo);
>      g_unlink(out);
> @@ -1371,7 +1399,7 @@ static int chardev_change_denied(void *opaque)
>
>  static void char_hotswap_test(void)
>  {
> -    char *chr_args;
> +    char *chr_args = NULL;
>      Chardev *chr;
>      CharBackend be;
>
> @@ -1385,6 +1413,9 @@ static void char_hotswap_test(void)
>      int port;
>      int sock = make_udp_socket(&port);
>      g_assert_cmpint(sock, >, 0);
> +    if (sock < 0) {
> +        goto fail;
> +    }
>
>      chr_args = g_strdup_printf("udp:127.0.0.1:%d", port);
>
> @@ -1422,6 +1453,7 @@ static void char_hotswap_test(void)
>      object_unparent(OBJECT(chr));
>
>      qapi_free_ChardevReturn(ret);
> +fail:
>      g_unlink(filename);
>      g_free(filename);
>      g_rmdir(tmp_path);
> --
> 2.20.1
>
>

On Tue, 8 Jun 2021 at 20:51, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Tue, Jun 8, 2021 at 9:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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.

The point is that I want to make them happen, though...

> I would teach coverity instead that those asserts are always fatal.

If you want an assert that's always fatal, that's g_assert().
These ones are documented as not always fatal.

> Fwiw, none of the tests in glib or gtk seem to use
> g_test_set_nonfatal_assertions(), probably for similar considerations.

That's interesting. I did wonder about these APIs, and if glib
themselves aren't using them that seems like a reason why they're
so awkward.

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 8 Jun 2021 at 20:51, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>>
>> Hi
>>
>> On Tue, Jun 8, 2021 at 9:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 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.
>
> The point is that I want to make them happen, though...

I'd prefer not to.

Writing tests is tedious enough as it is.  Replacing

    assert COND in one of the many ways GLib provides

by

    assert COND in one of the many ways GLib provides
    if (!COND) {
        bail out
    }

makes it worse.

Readability suffers, too.

>> I would teach coverity instead that those asserts are always fatal.
>
> If you want an assert that's always fatal, that's g_assert().
> These ones are documented as not always fatal.

You'd sacrifice the additional output from g_assert_cmpint() & friends,
which can sometimes save a trip through the debugger.  I don't care all
that much myself, but I know others do.

>> Fwiw, none of the tests in glib or gtk seem to use
>> g_test_set_nonfatal_assertions(), probably for similar considerations.
>
> That's interesting. I did wonder about these APIs, and if glib
> themselves aren't using them that seems like a reason why they're
> so awkward.

Plain assert()'s behavior is configurable at compile time: assertion
checking on / off.  This sets a trap for the unwary: side effects in the
argument.  We avoid the trap by gluing the compile-time switch to "on".

GLib's optionally non-fatal assertions add new traps, with much less
excuse.  Without recovery code, non-fatal assertions make little sense.
But when you have to add recovery code anyway, you could easily switch
to a new set of check functions, too.  Overloading the existing
assertion functions was in bad taste.

On Wed, 9 Jun 2021 at 13:36, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 8 Jun 2021 at 20:51, Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> >>
> >> Hi
> >>
> >> On Tue, Jun 8, 2021 at 9:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> 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.
> >
> > The point is that I want to make them happen, though...
>
> I'd prefer not to.
>
> Writing tests is tedious enough as it is.  Replacing
>
>     assert COND in one of the many ways GLib provides
>
> by
>
>     assert COND in one of the many ways GLib provides
>     if (!COND) {
>         bail out
>     }
>
> makes it worse.
>
> Readability suffers, too.

I agree. But glib doesn't provide a "check this test thing I'm
trying to test, and make it cleanly abandon and fail the test
if the check passes" function. I suppose we could rig one up
with setjmp/longjmp and some macros...

> >> I would teach coverity instead that those asserts are always fatal.
> >
> > If you want an assert that's always fatal, that's g_assert().
> > These ones are documented as not always fatal.
>
> You'd sacrifice the additional output from g_assert_cmpint() & friends,
> which can sometimes save a trip through the debugger.  I don't care all
> that much myself, but I know others do.

> Plain assert()'s behavior is configurable at compile time: assertion
> checking on / off.  This sets a trap for the unwary: side effects in the
> argument.  We avoid the trap by gluing the compile-time switch to "on".
>
> GLib's optionally non-fatal assertions add new traps, with much less
> excuse.  Without recovery code, non-fatal assertions make little sense.
> But when you have to add recovery code anyway, you could easily switch
> to a new set of check functions, too.  Overloading the existing
> assertion functions was in bad taste.

I agree that I wouldn't have named them _assert myself...

-- PMM