Re: [PATCH v2 3/6] tests/qtest: capture RESUME events during migration

[Daniel P . Berrangé]

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