| Message ID | 20210917054047.2042843-5-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Switch iotests to using Async QMP | expand |
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”? (Also, perhaps renaming it to get_new_events, but that’s an even weaker suggestion.) Anyway: Reviewed-by: Hanna Reitz <hreitz@redhat.com>
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 ~
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index 34131884a5..ae945ca3c9 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -631,7 +631,6 @@ def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]: events = self._qmp.get_events(wait=wait) events.extend(self._events) del self._events[:] - self._qmp.clear_events() return events @staticmethod 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 def clear_events(self) -> None: """ diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py index 337acfce2d..e7d7eb18f1 100644 --- a/python/qemu/qmp/qmp_shell.py +++ b/python/qemu/qmp/qmp_shell.py @@ -381,7 +381,6 @@ def read_exec_command(self) -> bool: if cmdline == '': for event in self.get_events(): print(event) - self.clear_events() return True return self._execute_cmd(cmdline)
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(-)