tests/unit/check-block-qdict.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
In CID 1432593, Coverity complains that the result of qdict_crumple()
might leak if it is not a dictionary. This is not a practical concern
since the test would fail immediately with a NULL pointer dereference
in qdict_size().
However, it is not nice to depend on qdict_size() crashing, so add an
explicit assertion that that the crumpled object was indeed a dictionary.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
tests/unit/check-block-qdict.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/unit/check-block-qdict.c b/tests/unit/check-block-qdict.c
index 5a25825093..751c58e737 100644
--- a/tests/unit/check-block-qdict.c
+++ b/tests/unit/check-block-qdict.c
@@ -504,7 +504,7 @@ static void qdict_crumple_test_empty(void)
src = qdict_new();
dst = qobject_to(QDict, qdict_crumple(src, &error_abort));
-
+ g_assert(dst);
g_assert_cmpint(qdict_size(dst), ==, 0);
qobject_unref(src);
--
2.37.2
Paolo Bonzini <pbonzini@redhat.com> writes: > In CID 1432593, Coverity complains that the result of qdict_crumple() > might leak if it is not a dictionary. This is not a practical concern > since the test would fail immediately with a NULL pointer dereference > in qdict_size(). > > However, it is not nice to depend on qdict_size() crashing, so add an > explicit assertion that that the crumpled object was indeed a dictionary. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tests/unit/check-block-qdict.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/unit/check-block-qdict.c b/tests/unit/check-block-qdict.c > index 5a25825093..751c58e737 100644 > --- a/tests/unit/check-block-qdict.c > +++ b/tests/unit/check-block-qdict.c > @@ -504,7 +504,7 @@ static void qdict_crumple_test_empty(void) > src = qdict_new(); > > dst = qobject_to(QDict, qdict_crumple(src, &error_abort)); > - > + g_assert(dst); > g_assert_cmpint(qdict_size(dst), ==, 0); > > qobject_unref(src); First, I'm fine with the patch, so Reviewed-by: Markus Armbruster <armbru@redhat.com> Next, permit me a few words on writing tests. For me, a unit test fails by crashing. Crashing with a nice message is optional. The more likely the failure, the more useful is niceness. Complete niceness is impossible --- if we could predict all crashes, we wouldn't need tests. Trying to push niceness can be overly verbose. Thus, judgement calls, and matters of taste. Wanting to mollify Coverity is a valid argument.
Il mar 6 set 2022, 07:01 Markus Armbruster <armbru@redhat.com> ha scritto: > Next, permit me a few words on writing tests. For me, a unit test fails > by crashing. Crashing with a nice message is optional. The more likely > the failure, the more useful is niceness. Complete niceness is > impossible --- if we could predict all crashes, we wouldn't need tests. > Trying to push niceness can be overly verbose. Thus, judgement calls, > and matters of taste. > I agree; however, *relying* on a crash for correctness of the test is not great. Part of the test here is checking that an empty qdict_crumple returns a dictionary and not, say, a list. The newly-added assertion avoids that two wrongs end up making a right: if qobject_check_type somehow failed to identify the dictionary and returned (QDict *) obj, qdict_size would not crash. Unlikely as it is, it's nicer to spell out the postconditions that the test is checking. Paolo > Wanting to mollify Coverity is a valid argument. > >
Paolo Bonzini <pbonzini@redhat.com> writes: > In CID 1432593, Coverity complains that the result of qdict_crumple() > might leak if it is not a dictionary. This is not a practical concern > since the test would fail immediately with a NULL pointer dereference > in qdict_size(). > > However, it is not nice to depend on qdict_size() crashing, so add an > explicit assertion that that the crumpled object was indeed a dictionary. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
© 2016 - 2024 Red Hat, Inc.