| Message ID | 20230421171411.566300-4-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest: make migration-test massively faster | expand |
Daniel P. Berrangé <berrange@redhat.com> wrote: > When running migration tests we monitor for a STOP event so we can skip > redundant waits. This will be needed for the RESUME event too shortly. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> i.e. it is better that what we have now. But > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c > index f6f3c6680f..61396335cc 100644 > --- a/tests/qtest/migration-helpers.c > +++ b/tests/qtest/migration-helpers.c > @@ -24,14 +24,20 @@ > #define MIGRATION_STATUS_WAIT_TIMEOUT 120 > > bool got_stop; > +bool got_resume; > > -static void check_stop_event(QTestState *who) > +static void check_events(QTestState *who) > { > QDict *event = qtest_qmp_event_ref(who, "STOP"); > if (event) { > got_stop = true; > qobject_unref(event); > } > + event = qtest_qmp_event_ref(who, "RESUME"); > + if (event) { > + got_resume = true; > + qobject_unref(event); > + } > } What happens if we receive the events in the order RESUME/STOP (I mean in the big scheme of things, not that it makes sense in this particular case). QDict *qtest_qmp_event_ref(QTestState *s, const char *event) { while (s->pending_events) { GList *first = s->pending_events; QDict *response = (QDict *)first->data; s->pending_events = g_list_delete_link(s->pending_events, first); if (!strcmp(qdict_get_str(response, "event"), event)) { return response; } qobject_unref(response); } return NULL; } if we don't found the event that we are searching for, we just drop it. Does this makes sense if we are searching only for more than one event? Later, Juan.
On Fri, Apr 21, 2023 at 11:59:25PM +0200, Juan Quintela wrote: > Daniel P. Berrangé <berrange@redhat.com> wrote: > > When running migration tests we monitor for a STOP event so we can skip > > redundant waits. This will be needed for the RESUME event too shortly. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > i.e. it is better that what we have now. > > But > > > > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c > > index f6f3c6680f..61396335cc 100644 > > --- a/tests/qtest/migration-helpers.c > > +++ b/tests/qtest/migration-helpers.c > > @@ -24,14 +24,20 @@ > > #define MIGRATION_STATUS_WAIT_TIMEOUT 120 > > > > bool got_stop; > > +bool got_resume; > > > > -static void check_stop_event(QTestState *who) > > +static void check_events(QTestState *who) > > { > > QDict *event = qtest_qmp_event_ref(who, "STOP"); > > if (event) { > > got_stop = true; > > qobject_unref(event); > > } > > + event = qtest_qmp_event_ref(who, "RESUME"); > > + if (event) { > > + got_resume = true; > > + qobject_unref(event); > > + } > > } > > What happens if we receive the events in the order RESUME/STOP (I mean > in the big scheme of things, not that it makes sense in this particular > case). > > QDict *qtest_qmp_event_ref(QTestState *s, const char *event) > { > while (s->pending_events) { > > GList *first = s->pending_events; > QDict *response = (QDict *)first->data; > > s->pending_events = g_list_delete_link(s->pending_events, first); > > if (!strcmp(qdict_get_str(response, "event"), event)) { > return response; > } > qobject_unref(response); > } > return NULL; > } > > if we don't found the event that we are searching for, we just drop it. > Does this makes sense if we are searching only for more than one event? You are right about this code being broken in general for multiple events. In this particular series though we're looking at STOP on the src host and RESUME on the dst host, so there's no ordering problem to worry about. With regards, Daniel
On Mon, Apr 24, 2023 at 10:53:36AM +0100, Daniel P. Berrangé wrote: > On Fri, Apr 21, 2023 at 11:59:25PM +0200, Juan Quintela wrote: > > Daniel P. Berrangé <berrange@redhat.com> wrote: > > > When running migration tests we monitor for a STOP event so we can skip > > > redundant waits. This will be needed for the RESUME event too shortly. > > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > > > i.e. it is better that what we have now. > > > > But > > > > > > > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c > > > index f6f3c6680f..61396335cc 100644 > > > --- a/tests/qtest/migration-helpers.c > > > +++ b/tests/qtest/migration-helpers.c > > > @@ -24,14 +24,20 @@ > > > #define MIGRATION_STATUS_WAIT_TIMEOUT 120 > > > > > > bool got_stop; > > > +bool got_resume; > > > > > > -static void check_stop_event(QTestState *who) > > > +static void check_events(QTestState *who) > > > { > > > QDict *event = qtest_qmp_event_ref(who, "STOP"); > > > if (event) { > > > got_stop = true; > > > qobject_unref(event); > > > } > > > + event = qtest_qmp_event_ref(who, "RESUME"); > > > + if (event) { > > > + got_resume = true; > > > + qobject_unref(event); > > > + } > > > } > > > > What happens if we receive the events in the order RESUME/STOP (I mean > > in the big scheme of things, not that it makes sense in this particular > > case). > > > > QDict *qtest_qmp_event_ref(QTestState *s, const char *event) > > { > > while (s->pending_events) { > > > > GList *first = s->pending_events; > > QDict *response = (QDict *)first->data; > > > > s->pending_events = g_list_delete_link(s->pending_events, first); > > > > if (!strcmp(qdict_get_str(response, "event"), event)) { > > return response; > > } > > qobject_unref(response); > > } > > return NULL; > > } > > > > if we don't found the event that we are searching for, we just drop it. > > Does this makes sense if we are searching only for more than one event? > > You are right about this code being broken in general for multiple events. > > In this particular series though we're looking at STOP on the src host and > RESUME on the dst host, so there's no ordering problem to worry about. What I wrote is nonsense. This is broken, because on the dst host, check_events will look for STOP and discard all remaining events, so we'll never find the RESUME that we are looking for. qtest_qmp_event_ref is essentially broken by design right now and I'll need to fix it. With regards, Daniel
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c index f6f3c6680f..61396335cc 100644 --- a/tests/qtest/migration-helpers.c +++ b/tests/qtest/migration-helpers.c @@ -24,14 +24,20 @@ #define MIGRATION_STATUS_WAIT_TIMEOUT 120 bool got_stop; +bool got_resume; -static void check_stop_event(QTestState *who) +static void check_events(QTestState *who) { QDict *event = qtest_qmp_event_ref(who, "STOP"); if (event) { got_stop = true; qobject_unref(event); } + event = qtest_qmp_event_ref(who, "RESUME"); + if (event) { + got_resume = true; + qobject_unref(event); + } } #ifndef _WIN32 @@ -48,7 +54,7 @@ QDict *wait_command_fd(QTestState *who, int fd, const char *command, ...) va_end(ap); resp = qtest_qmp_receive(who); - check_stop_event(who); + check_events(who); g_assert(!qdict_haskey(resp, "error")); g_assert(qdict_haskey(resp, "return")); @@ -73,7 +79,7 @@ QDict *wait_command(QTestState *who, const char *command, ...) resp = qtest_vqmp(who, command, ap); va_end(ap); - check_stop_event(who); + check_events(who); g_assert(!qdict_haskey(resp, "error")); g_assert(qdict_haskey(resp, "return")); diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h index a188b62787..726a66cfc1 100644 --- a/tests/qtest/migration-helpers.h +++ b/tests/qtest/migration-helpers.h @@ -16,6 +16,7 @@ #include "libqtest.h" extern bool got_stop; +extern bool got_resume; #ifndef _WIN32 G_GNUC_PRINTF(3, 4) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index e16120ff30..6492ffa7fe 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -596,6 +596,7 @@ static int test_migrate_start(QTestState **from, QTestState **to, } got_stop = false; + got_resume = false; bootpath = g_strdup_printf("%s/bootsect", tmpfs); if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { /* the assembled x86 boot sector should be exactly one sector large */
When running migration tests we monitor for a STOP event so we can skip redundant waits. This will be needed for the RESUME event too shortly. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tests/qtest/migration-helpers.c | 12 +++++++++--- tests/qtest/migration-helpers.h | 1 + tests/qtest/migration-test.c | 1 + 3 files changed, 11 insertions(+), 3 deletions(-)