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

From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Thomas Huth <thuth@redhat.com>
Cc: qemu-devel@nongnu.org, Laurent Vivier <lvivier@redhat.com>,
	Peter Xu <peterx@redhat.com>, Juan Quintela <quintela@redhat.com>,
	Leonardo Bras <leobras@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v3 1/9] tests/qtest: add various qtest_qmp_assert_success() variants
Date: Thu, 1 Jun 2023 13:48:49 +0100	[thread overview]
Message-ID: <ZHiTsRKrG24QojEM@redhat.com> (raw)
In-Reply-To: <629f6745-6c36-93a9-8ebe-a8a83fd154c7@redhat.com>

On Thu, Jun 01, 2023 at 11:23:01AM +0200, Thomas Huth wrote:
> 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(-)

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

Sure, will do.

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

Agreed

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