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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Jun 08, 2021 at 06:06:06PM +0100, Peter Maydell 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...

In that thread you show a difference in the TAP output when
g_test_set_nonfatal_assertions is enabled. Instead of it
reporting an abort, it reports an error against the test
and carries on running.

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

Yes, it is a bit of a philosophical question which behaviour
is better - immediate exit, vs report & carry on.  In the
Perl world the normal is to report & carry on so you get
full results for the entire suite. In python / C world it
has been more common to immediately exit.

The report & carry on obviously results in cascading errors
unless you take extra steps to skip stuff you know is going
to cascade. You did some examples of that here with the extra
'goto fail' jumps.

The flipside is that if you have a test that fails 6
different scenarios it is nice to see all 6 failures upfront,
instead of having to play whack-a-mole fixing one and then
discovering the next failure, then fixing that and discovering
the next failure, etc.


When we discussed this last on IRC, I suggested that we
introduce a 'q_test_init' that wraps around g_test_init.
This q_test_init could set g_test_set_nonfatal_assertions
and call 'g_test_init'.

This would avoid need for coccinelle script, as a sed
s/g_test_init/q_test_init/ would suffice. We can stuff
other logic into q_test_Init if we wanted to. Perhaps
a private TMPDIR for example.

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

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