| Message ID | 20230531132400.1129576-4-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest: make migration-test massively faster | expand |
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. Thomas
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
Daniel P. Berrangé <berrange@redhat.com> wrote: > This function duplicates logic of qtest_qmp_assert_success_ref > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Much better that the current code. And as you answer to Thomas, better messages in case of errors.
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")); - g_assert(qdict_haskey(resp, "return")); - - ret = qdict_get_qdict(resp, "return"); - qobject_ref(ret); - qobject_unref(resp); - - return ret; -} - /* * Send QMP command "migrate". * Arguments are built from @fmt... (formatted like diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h index a188b62787..2e51a6e195 100644 --- a/tests/qtest/migration-helpers.h +++ b/tests/qtest/migration-helpers.h @@ -25,9 +25,6 @@ QDict *wait_command_fd(QTestState *who, int fd, const char *command, ...); G_GNUC_PRINTF(2, 3) QDict *wait_command(QTestState *who, const char *command, ...); -G_GNUC_PRINTF(2, 3) -QDict *qmp_command(QTestState *who, const char *command, ...); - G_GNUC_PRINTF(3, 4) void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...); diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index b99b49a314..9ce27f89ec 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -2322,32 +2322,33 @@ static void test_multifd_tcp_cancel(void) static void calc_dirty_rate(QTestState *who, uint64_t calc_time) { - qobject_unref(qmp_command(who, - "{ 'execute': 'calc-dirty-rate'," - "'arguments': { " - "'calc-time': %" PRIu64 "," - "'mode': 'dirty-ring' }}", - calc_time)); + qtest_qmp_assert_success(who, + "{ 'execute': 'calc-dirty-rate'," + "'arguments': { " + "'calc-time': %" PRIu64 "," + "'mode': 'dirty-ring' }}", + calc_time); } static QDict *query_dirty_rate(QTestState *who) { - return qmp_command(who, "{ 'execute': 'query-dirty-rate' }"); + return qtest_qmp_assert_success_ref(who, + "{ 'execute': 'query-dirty-rate' }"); } static void dirtylimit_set_all(QTestState *who, uint64_t dirtyrate) { - qobject_unref(qmp_command(who, - "{ 'execute': 'set-vcpu-dirty-limit'," - "'arguments': { " - "'dirty-rate': %" PRIu64 " } }", - dirtyrate)); + qtest_qmp_assert_success(who, + "{ 'execute': 'set-vcpu-dirty-limit'," + "'arguments': { " + "'dirty-rate': %" PRIu64 " } }", + dirtyrate); } static void cancel_vcpu_dirty_limit(QTestState *who) { - qobject_unref(qmp_command(who, - "{ 'execute': 'cancel-vcpu-dirty-limit' }")); + qtest_qmp_assert_success(who, + "{ 'execute': 'cancel-vcpu-dirty-limit' }"); } static QDict *query_vcpu_dirty_limit(QTestState *who)
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(-)