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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6586 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 8341 bytes --]