Re: [PATCH v3 3/9] tests/qtest: get rid of 'qmp_command' helper in migration test

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 3/9] tests/qtest: get rid of 'qmp_command' helper in migration test
Date: Thu, 1 Jun 2023 10:32:42 +0100	[thread overview]
Message-ID: <ZHhluk6cApCygV4S@redhat.com> (raw)
In-Reply-To: <7e0f528c-ba81-068c-46b6-e0f3142ec1b8@redhat.com>

On Thu, Jun 01, 2023 at 11:26:46AM +0200, Thomas Huth wrote:
> On 31/05/2023 15.23, Daniel P. Berrangé wrote:
> > This function duplicates logic of qtest_qmp_assert_success_ref
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/qtest/migration-helpers.c | 22 ----------------------
> >   tests/qtest/migration-helpers.h |  3 ---
> >   tests/qtest/migration-test.c    | 29 +++++++++++++++--------------
> >   3 files changed, 15 insertions(+), 39 deletions(-)
> > 
> > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> > index f6f3c6680f..bddf3f8d4d 100644
> > --- a/tests/qtest/migration-helpers.c
> > +++ b/tests/qtest/migration-helpers.c
> > @@ -85,28 +85,6 @@ QDict *wait_command(QTestState *who, const char *command, ...)
> >       return ret;
> >   }
> > -/*
> > - * Execute the qmp command only
> > - */
> > -QDict *qmp_command(QTestState *who, const char *command, ...)
> > -{
> > -    va_list ap;
> > -    QDict *resp, *ret;
> > -
> > -    va_start(ap, command);
> > -    resp = qtest_vqmp(who, command, ap);
> > -    va_end(ap);
> > -
> > -    g_assert(!qdict_haskey(resp, "error"));
> 
> What about this g_assert(!qdict_haskey(resp, "error")) ?
> qtest_qmp_assert_success_ref() does not have this assert... do we still need
> it somewhere? If not, please add a comment to the patch description why it
> can be ignored now.

The caller just wants the 'return' field data. If that is missing,
qtest_qmp_assert_success_ref() will issue the diagnostic message
printing the entire QMP resposne, which is way more debuggable
than just asserting on 'error' without printing the error contents

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