Re: [PATCH 04/15] python/qmp: clear events on get_events() call

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2799 bytes --]

On Fri, Sep 17, 2021 at 8:51 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 17.09.21 07:40, John Snow wrote:
> > All callers in the tree *already* clear the events after a call to
> > get_events(). Do it automatically instead and update callsites to remove
> > the manual clear call.
> >
> > These semantics are quite a bit easier to emulate with async QMP, and
> > nobody appears to be abusing some emergent properties of what happens if
> > you decide not to clear them, so let's dial down to the dumber, simpler
> > thing.
> >
> > Specifically: callers of clear() right after a call to get_events() are
> > more likely expressing their desire to not see any events they just
> > retrieved, whereas callers of clear_events() not in relation to a recent
> > call to pull_event/get_events are likely expressing their desire to
> > simply drop *all* pending events straight onto the floor. In the sync
> > world, this is safe enough; in the async world it's nearly impossible to
> > promise that nothing happens between getting and clearing the
> > events.
> >
> > Making the retrieval also clear the queue is vastly simpler.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   python/qemu/machine/machine.py | 1 -
> >   python/qemu/qmp/__init__.py    | 4 +++-
> >   python/qemu/qmp/qmp_shell.py   | 1 -
> >   3 files changed, 3 insertions(+), 3 deletions(-)
>
> [...]
>
> > diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
> > index 269516a79b..ba15668c25 100644
> > --- a/python/qemu/qmp/__init__.py
> > +++ b/python/qemu/qmp/__init__.py
> > @@ -374,7 +374,9 @@ def get_events(self, wait: bool = False) ->
> List[QMPMessage]:
> >           @return The list of available QMP events.
> >           """
> >           self.__get_events(wait)
> > -        return self.__events
> > +        events = self.__events
> > +        self.__events = []
> > +        return events
>
> Maybe it’s worth updating the doc string that right now just says “Get a
> list of available QMP events”?
>
>
Yes, that's an oversight on my part. I'm updating it to:
"Get a list of available QMP events and clear the list of pending events."
and adding your RB.

(Also, perhaps renaming it to get_new_events, but that’s an even weaker
> suggestion.)
>

I tried to avoid churn in the iotests as much as I could manage, so I think
I will leave this be for now. I have an impression that the number of cases
where one might wish to see events that have already been witnessed is
actually quite low, so I am not sure that it needs disambiguation from a
case that is essentially never used. (I have certainly been wrong before,
though.)


> Anyway:
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>
~ thankyou ~

[-- Attachment #2: Type: text/html, Size: 3913 bytes --]