Re: [PATCH v3 1/9] tests/qtest: add various qtest_qmp_assert_success() variants

[Thomas Huth]

On 31/05/2023 15.23, Daniel P. Berrangé wrote:
> Add several counterparts of qtest_qmp_assert_success() that can
>
>   * Use va_list instead of ...
>   * Accept a list of FDs to send
>   * Return the response data
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qtest/libqtest.c |  99 +++++++++++++++++++++++++++++++++--
>   tests/qtest/libqtest.h | 115 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 209 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index c3a0ef5bb4..603c26d955 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -1229,14 +1229,23 @@ void qtest_memset(QTestState *s, uint64_t addr, uint8_t 
> pattern, size_t size)
>       qtest_rsp(s);
>   }
>   
> -void qtest_qmp_assert_success(QTestState *qts, const char *fmt, ...)
> +void qtest_vqmp_assert_success(QTestState *qts,
> +                               const char *fmt, va_list args)
>   {
> -    va_list ap;
>       QDict *response;
>   
> -    va_start(ap, fmt);
> -    response = qtest_vqmp(qts, fmt, ap);
> -    va_end(ap);
> +    response = qtest_vqmp_assert_success_ref(qts, fmt, args);
> +
> +    qobject_unref(response);
> +}
> +
> +QDict *qtest_vqmp_assert_success_ref(QTestState *qts,
> +                                     const char *fmt, va_list args)
> +{
> +    QDict *response;
> +    QDict *ret;
> +
> +    response = qtest_vqmp(qts, fmt, args);
>   
>       g_assert(response);
>       if (!qdict_haskey(response, "return")) {
> @@ -1245,8 +1254,88 @@ void qtest_qmp_assert_success(QTestState *qts, const 
> char *fmt, ...)
>           g_string_free(s, true);
>       }
>       g_assert(qdict_haskey(response, "return"));
> +    ret = qdict_get_qdict(response, "return");
> +    qobject_ref(ret);
> +    qobject_unref(response);
> +
> +    return ret;
> +}
> +
> +#ifndef _WIN32
> +QDict *qtest_vqmp_fds_assert_success_ref(QTestState *qts, int *fds, size_t 
> nfds,
> +                                         const char *fmt, va_list args)
> +{
> +    QDict *response;
> +    QDict *ret;
> +
> +    response = qtest_vqmp_fds(qts, fds, nfds, fmt, args);
> +
> +    g_assert(response);
> +    if (!qdict_haskey(response, "return")) {
> +        GString *s = qobject_to_json_pretty(QOBJECT(response), true);
> +        g_test_message("%s", s->str);
> +        g_string_free(s, true);
> +    }
> +    g_assert(qdict_haskey(response, "return"));
> +    ret = qdict_get_qdict(response, "return");
> +    qobject_ref(ret);
> +    qobject_unref(response);
> +
> +    return ret;
> +}
> +
> +void qtest_vqmp_fds_assert_success(QTestState *qts, int *fds, size_t nfds,
> +                                   const char *fmt, va_list args)
> +{
> +    QDict *response;
> +    response = qtest_vqmp_fds_assert_success_ref(qts, fds, nfds, fmt, args);
>       qobject_unref(response);
>   }

<bikeshedding>The ordering is a little bit inconsistent ... for some pairs, 
you do the _success() function first, and for some others you do the 
_success_ref() function first. IMHO it would be nicer to have the same 
ordering everywhere, preferably with the _success_ref() function first 
(since that's the one that is called from the other).</bikeshedding>

> +#endif /* !_WIN32 */
> +
> +void qtest_qmp_assert_success(QTestState *qts, const char *fmt, ...)
> +{
> +    QDict *response;
> +    va_list ap;
> +    va_start(ap, fmt);
> +    response = qtest_vqmp_assert_success_ref(qts, fmt, ap);

You could use qtest_vqmp_assert_success() instead, I think, and dro the 
qobject_unref() below.

> +    va_end(ap);
> +    qobject_unref(response);
> +}
> +
> +QDict *qtest_qmp_assert_success_ref(QTestState *qts, const char *fmt, ...)
> +{
> +    QDict *response;
> +    va_list ap;
> +    va_start(ap, fmt);
> +    response = qtest_vqmp_assert_success_ref(qts, fmt, ap);
> +    va_end(ap);
> +    return response;
> +}
> +
> +#ifndef _WIN32
> +void qtest_qmp_fds_assert_success(QTestState *qts, int *fds, size_t nfds,
> +                                  const char *fmt, ...)
> +{
> +    QDict *response;
> +    va_list ap;
> +    va_start(ap, fmt);
> +    response = qtest_vqmp_fds_assert_success_ref(qts, fds, nfds, fmt, ap);

dito, could use qtest_vqmp_fds_assert_success() instead.

> +    va_end(ap);
> +    qobject_unref(response);
> +}
> +
> +QDict *qtest_qmp_fds_assert_success_ref(QTestState *qts, int *fds, size_t nfds,
> +                                        const char *fmt, ...)
> +{
> +    QDict *response;
> +    va_list ap;
> +    va_start(ap, fmt);
> +    response = qtest_vqmp_fds_assert_success_ref(qts, fds, nfds, fmt, ap);
> +    va_end(ap);
> +    return response;
> +}
> +#endif /* !_WIN32 */

Just nits, so anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>