Message ID | 20201120073149.99079-1-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
Series | qtest: do not return freed argument vector from qtest_rsp | expand |
On 20/11/2020 08.31, Paolo Bonzini wrote: > If expected_args is 0, qtest frees the argument vector and then returns it > nevertheless. Coverity complains; in practice this is not an issue because > expected_args == 0 means that the caller is not interested in the argument > vector, but it would be a potential problem if somebody wanted to add > commands with optional arguments to qtest. > > Suggested-by: Kamil Dudka <kdudka@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tests/qtest/libqtest.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c > index be0fb430dd..e49f3a1e45 100644 > --- a/tests/qtest/libqtest.c > +++ b/tests/qtest/libqtest.c > @@ -545,6 +545,7 @@ redo: > } > } else { > g_strfreev(words); > + words = NULL; > } > > return words; > Reviewed-by: Thomas Huth <thuth@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes: > If expected_args is 0, qtest frees the argument vector and then returns it > nevertheless. Coverity complains; in practice this is not an issue because > expected_args == 0 means that the caller is not interested in the argument > vector, but it would be a potential problem if somebody wanted to add > commands with optional arguments to qtest. > > Suggested-by: Kamil Dudka <kdudka@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tests/qtest/libqtest.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c > index be0fb430dd..e49f3a1e45 100644 > --- a/tests/qtest/libqtest.c > +++ b/tests/qtest/libqtest.c > @@ -545,6 +545,7 @@ redo: if (expected_args) { for (i = 0; i < expected_args; i++) { g_assert(words[i] != NULL); > } > } else { > g_strfreev(words); > + words = NULL; > } > > return words; The patch is okay, so Reviewed-by: Markus Armbruster <armbru@redhat.com> Odd: we require at least @expected_args, but more is fine. Not what the name @expected_args made me expect. We treat expected_args == 0 as special case to save callers the trouble to free @words. Okay, but I'd try a more regular interface: * one function that takes @expected_args and returns @words, always. * a simple wrapper around it that passes zero @expected_args and frees @words.
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index be0fb430dd..e49f3a1e45 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -545,6 +545,7 @@ redo: } } else { g_strfreev(words); + words = NULL; } return words;
If expected_args is 0, qtest frees the argument vector and then returns it nevertheless. Coverity complains; in practice this is not an issue because expected_args == 0 means that the caller is not interested in the argument vector, but it would be a potential problem if somebody wanted to add commands with optional arguments to qtest. Suggested-by: Kamil Dudka <kdudka@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- tests/qtest/libqtest.c | 1 + 1 file changed, 1 insertion(+)