On Fri, Sep 17, 2021 at 8:51 AM Hanna Reitz <hre...@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 <js...@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 <hre...@redhat.com> > > ~ thankyou ~
