Re: [PATCH v3 2/9] tests/qtest: add support for callback to receive QMP events

From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Juan Quintela <quintela@redhat.com>
Cc: qemu-devel@nongnu.org, Laurent Vivier <lvivier@redhat.com>,
	Peter Xu <peterx@redhat.com>, Leonardo Bras <leobras@redhat.com>,
	Thomas Huth <thuth@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v3 2/9] tests/qtest: add support for callback to receive QMP events
Date: Thu, 1 Jun 2023 13:56:14 +0100	[thread overview]
Message-ID: <ZHiVbkRkTFCqIzXg@redhat.com> (raw)
In-Reply-To: <87zg5js7lb.fsf@secure.mitica>

On Thu, Jun 01, 2023 at 02:14:08PM +0200, Juan Quintela wrote:
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > Currently code must call one of the qtest_qmp_event* functions to
> > fetch events. These are only usable if the immediate caller knows
> > the particular event they want to capture, and are only interested
> > in one specific event type. Adding ability to register an event
> > callback lets the caller capture a range of events over any period
> > of time.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> First of all, I *love* the idea of the patch, but ...
> 
> >  static GHookList abrt_hooks;
> > @@ -703,8 +705,14 @@ QDict *qtest_qmp_receive(QTestState *s)
> >          if (!qdict_get_try_str(response, "event")) {
> >              return response;
> >          }
> > -        /* Stash the event for a later consumption */
> > -        s->pending_events = g_list_append(s->pending_events, response);
> > +
> > +        if (s->eventCB) {
> > +            s->eventCB(s, qdict_get_str(response, "event"),
> > +                       response, s->eventData);
> > +        } else {
> > +            /* Stash the event for a later consumption */
> > +            s->pending_events = g_list_append(s->pending_events, response);
> > +        }
> >      }
> 
> s->eventCB returns a bool that you are not using.  I think this part of
> the code would be more usefule if:
> 
>         if (!s->eventCB || !s->eventCB(s, qdict_get_str(response, "event"),
>                                        response, s->eventData)) {
>             /* Stash the event for a later consumption */
>             s->pending_events = g_list_append(s->pending_events, response);
>         }
> 
> So if we are not handling the event, we put it on the pending_events
> list.
> 
> What do you think?

Sure, it is easy enough to do.

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