| Message ID | 20210608170607.21902-1-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tests/unit/test-char.c: Fix error handling issues | expand |
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 > > <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 8, 2021 at 9:06 PM Peter Maydell <<a href="mailto:peter.maydell@linaro.org">peter.maydell@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Coverity spots some minor error-handling issues in this test code.<br> These are mostly due to the test code assuming that the glib test<br> macros g_assert_cmpint() and friends will always abort on failure.<br> This is not the case: if the test case chooses to call<br> g_test_set_nonfatal_assertions() then they will mark the test case as<br> a failure and continue. (This is different to g_assert(),<br> g_assert_not_reached(), and assert(), which really do all always kill<br> the process.) The idea is that you use g_assert() for things<br> which are really assertions, as you would in normal QEMU code,<br> and g_assert_cmpint() and friends for "this check is the thing<br> this test case is testing" checks.<br> <br> In fact this test case does not currently set assertions to be<br> nonfatal, but we should ideally be aiming to get to a point where we<br> can set that more generally, because the test harness gives much<br> better error reporting (including minor details like "what was the<br> name of the test case that actually failed") than a raw assert/abort<br> does. So we mostly fix the Coverity nits by making the error-exit<br> path return early if necessary, rather than by converting the<br> g_assert_cmpint()s to g_assert()s.<br> <br> Fixes: Coverity CID 1432505, 1432514, 1432600, 1451384<br> Signed-off-by: Peter Maydell <<a href="mailto:peter.maydell@linaro.org" target="_blank">peter.maydell@linaro.org</a>><br> ---<br> We had some previous not-very-conclusive discussion about<br> g_assert_foo vs g_assert in this thread:<br> <a href="https://lore.kernel.org/qemu-devel/CAFEAcA9juOChqrh5orybJQwpQsyEZ5z3Dvmy7fjX0DW4Nbgmrg@mail.gmail.com/" rel="noreferrer" target="_blank">https://lore.kernel.org/qemu-devel/CAFEAcA9juOChqrh5orybJQwpQsyEZ5z3Dvmy7fjX0DW4Nbgmrg@mail.gmail.com/</a><br> This patch is in some sense me asserting my opinion about the<br> right thing to do. You might disagree...<br> <br> I think that improving the quality of the failure reporting<br> in 'make check' is useful, and that we should probably turn<br> on g_test_set_nonfatal_assertions() everywhere. (The worst that<br> can happen is that instead of crashing on the assert we proceed<br> and crash a bit later, I think.) Awkwardly we don't have a single<br> place where we could put that call, so I guess it's a coccinelle<br> script to add it to every test's main() function.<br> <br></blockquote><div><br></div><div>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.<br></div><div><br></div><div>Fwiw, none of the tests in glib or gtk seem to use g_test_set_nonfatal_assertions(), probably for similar considerations.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> tests/unit/test-char.c | 36 ++++++++++++++++++++++++++++++++++--<br> 1 file changed, 34 insertions(+), 2 deletions(-)<br> <br> diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c<br> index 5b3b48ebacd..43630ab57f8 100644<br> --- a/tests/unit/test-char.c<br> +++ b/tests/unit/test-char.c<br> @@ -214,6 +214,10 @@ static void char_mux_test(void)<br> qemu_chr_fe_take_focus(&chr_be2);<br> <br> base = qemu_chr_find("mux-label-base");<br> + g_assert_nonnull(base);<br> + if (base == 0) {<br> + goto fail;<br> + }<br> g_assert_cmpint(qemu_chr_be_can_write(base), !=, 0);<br> <br> qemu_chr_be_write(base, (void *)"hello", 6);<br> @@ -333,6 +337,7 @@ static void char_mux_test(void)<br> g_assert_cmpint(strlen(data), !=, 0);<br> g_free(data);<br> <br> +fail:<br> qemu_chr_fe_deinit(&chr_be1, false);<br> qemu_chr_fe_deinit(&chr_be2, true);<br> }<br> @@ -486,6 +491,9 @@ static void char_pipe_test(void)<br> chr = qemu_chr_new("pipe", tmp, NULL);<br> g_assert_nonnull(chr);<br> g_free(tmp);<br> + if (!chr) {<br> + goto fail;<br> + }<br> <br> qemu_chr_fe_init(&be, chr, &error_abort);<br> <br> @@ -493,12 +501,20 @@ static void char_pipe_test(void)<br> g_assert_cmpint(ret, ==, 9);<br> <br> fd = open(out, O_RDWR);<br> + g_assert_cmpint(fd, >=, 0);<br> + if (fd < 0) {<br> + goto fail;<br> + }<br> ret = read(fd, buf, sizeof(buf));<br> g_assert_cmpint(ret, ==, 9);<br> g_assert_cmpstr(buf, ==, "pipe-out");<br> close(fd);<br> <br> fd = open(in, O_WRONLY);<br> + g_assert_cmpint(fd, >=, 0);<br> + if (fd < 0) {<br> + goto fail;<br> + }<br> ret = write(fd, "pipe-in", 8);<br> g_assert_cmpint(ret, ==, 8);<br> close(fd);<br> @@ -518,6 +534,7 @@ static void char_pipe_test(void)<br> <br> qemu_chr_fe_deinit(&be, true);<br> <br> +fail:<br> g_assert(g_unlink(in) == 0);<br> g_assert(g_unlink(out) == 0);<br> g_assert(g_rmdir(tmp_path) == 0);<br> @@ -556,7 +573,10 @@ static int make_udp_socket(int *port)<br> socklen_t alen = sizeof(addr);<br> int ret, sock = qemu_socket(PF_INET, SOCK_DGRAM, 0);<br> <br> - g_assert_cmpint(sock, >, 0);<br> + g_assert_cmpint(sock, >=, 0);<br> + if (sock < 0) {<br> + return sock;<br> + }<br> addr.sin_family = AF_INET ;<br> addr.sin_addr.s_addr = htonl(INADDR_ANY);<br> addr.sin_port = 0;<br> @@ -586,6 +606,9 @@ static void char_udp_test_internal(Chardev *reuse_chr, int sock)<br> } else {<br> int port;<br> sock = make_udp_socket(&port);<br> + if (sock < 0) {<br> + return;<br> + }<br> tmp = g_strdup_printf("udp:127.0.0.1:%d", port);<br> chr = qemu_chr_new("client", tmp, NULL);<br> g_assert_nonnull(chr);<br> @@ -1224,6 +1247,10 @@ static void char_file_fifo_test(void)<br> }<br> <br> fd = open(fifo, O_RDWR);<br> + g_assert_cmpint(fd, >=, 0);<br> + if (fd < 0) {<br> + goto fail;<br> + }<br> ret = write(fd, "fifo-in", 8);<br> g_assert_cmpint(ret, ==, 8);<br> <br> @@ -1253,6 +1280,7 @@ static void char_file_fifo_test(void)<br> <br> qemu_chr_fe_deinit(&be, true);<br> <br> +fail:<br> g_unlink(fifo);<br> g_free(fifo);<br> g_unlink(out);<br> @@ -1371,7 +1399,7 @@ static int chardev_change_denied(void *opaque)<br> <br> static void char_hotswap_test(void)<br> {<br> - char *chr_args;<br> + char *chr_args = NULL;<br> Chardev *chr;<br> CharBackend be;<br> <br> @@ -1385,6 +1413,9 @@ static void char_hotswap_test(void)<br> int port;<br> int sock = make_udp_socket(&port);<br> g_assert_cmpint(sock, >, 0);<br> + if (sock < 0) {<br> + goto fail;<br> + }<br> <br> chr_args = g_strdup_printf("udp:127.0.0.1:%d", port);<br> <br> @@ -1422,6 +1453,7 @@ static void char_hotswap_test(void)<br> object_unparent(OBJECT(chr));<br> <br> qapi_free_ChardevReturn(ret);<br> +fail:<br> g_unlink(filename);<br> g_free(filename);<br> g_rmdir(tmp_path);<br> -- <br> 2.20.1<br> <br> </blockquote></div></div>
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
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 :|
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 :|
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
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);
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(-) -- 2.20.1