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é <berra...@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é <berra...@redhat.com>
> >
> > Reviewed-by: Juan Quintela <quint...@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
--
|: 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 :|
