| Message ID | 20201020172742.1483258-21-jsnow@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Python patches | expand |
On Tue, Oct 20, 2020 at 8:52 PM John Snow <jsnow@redhat.com> wrote: > > Nested if conditions don't change when the exception block fires; we > need to explicitly re-raise the error if we didn't intend to capture and > suppress it. > > Signed-off-by: John Snow <jsnow@redhat.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Message-id: 20201009175123.249009-3-jsnow@redhat.com > Signed-off-by: John Snow <jsnow@redhat.com> > --- > python/qemu/qmp.py | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py > index d911999da1..4969e5741c 100644 > --- a/python/qemu/qmp.py > +++ b/python/qemu/qmp.py > @@ -165,14 +165,15 @@ def __get_events(self, wait: Union[bool, float] = False) -> None: > """ > > # Check for new events regardless and pull them into the cache: > - self.__sock.setblocking(False) > try: > + self.__sock.setblocking(False) This change is not required. The idiom is: do stuff try: something finally: undo stuff If do stuff failed, there is no need to undo it. socket.setblocking() should not fail with EAGAIN, so it does not need to be inside the try block. > self.__json_read() > except OSError as err: > - if err.errno == errno.EAGAIN: > - # No data available > - pass > - self.__sock.setblocking(True) > + # EAGAIN: No data available; not critical > + if err.errno != errno.EAGAIN: > + raise In python 3 this can be simplified to: try: self.__json_read() except BlockingIOError: pass https://docs.python.org/3.6/library/exceptions.html#BlockingIOError > + finally: > + self.__sock.setblocking(True) > > # Wait for new events, if needed. > # if wait is 0.0, this means "no wait" and is also implicitly false. > -- > 2.26.2 Nir
On 10/20/20 2:15 PM, Nir Soffer wrote: > On Tue, Oct 20, 2020 at 8:52 PM John Snow <jsnow@redhat.com> wrote: >> >> Nested if conditions don't change when the exception block fires; we >> need to explicitly re-raise the error if we didn't intend to capture and >> suppress it. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Message-id: 20201009175123.249009-3-jsnow@redhat.com >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> python/qemu/qmp.py | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py >> index d911999da1..4969e5741c 100644 >> --- a/python/qemu/qmp.py >> +++ b/python/qemu/qmp.py >> @@ -165,14 +165,15 @@ def __get_events(self, wait: Union[bool, float] = False) -> None: >> """ >> >> # Check for new events regardless and pull them into the cache: >> - self.__sock.setblocking(False) >> try: >> + self.__sock.setblocking(False) > > This change is not required. The idiom is: > > do stuff > try: > something > finally: > undo stuff > > If do stuff failed, there is no need to undo it. > > socket.setblocking() should not fail with EAGAIN, so it > does not need to be inside the try block. > Squashing this change in, will send a new V2 cover letter. >> self.__json_read() >> except OSError as err: >> - if err.errno == errno.EAGAIN: >> - # No data available >> - pass >> - self.__sock.setblocking(True) >> + # EAGAIN: No data available; not critical >> + if err.errno != errno.EAGAIN: >> + raise > > In python 3 this can be simplified to: > > try: > self.__json_read() > except BlockingIOError: > pass > > https://docs.python.org/3.6/library/exceptions.html#BlockingIOError > I'm a lot less clear on this. We only check for EAGAIN, but that would check for EAGAIN, EALREADY, EWOULDBLOCK and EINPROGRESS. That's probably fine, really, but: There is something worse lurking in the code here too, and I really didn't want to get into it on this series, but we are making use of undefined behavior (sockfile.readline() on a non-blocking socket) -- It seems to work in practice so far, but it's begging to break. For that reason (This code should never have worked anyway), I am extremely reluctant to change the exception classes we catch here until we fix the problem. --js >> + finally: >> + self.__sock.setblocking(True) >> >> # Wait for new events, if needed. >> # if wait is 0.0, this means "no wait" and is also implicitly false. >> -- >> 2.26.2 > > Nir >
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py index d911999da1..4969e5741c 100644 --- a/python/qemu/qmp.py +++ b/python/qemu/qmp.py @@ -165,14 +165,15 @@ def __get_events(self, wait: Union[bool, float] = False) -> None: """ # Check for new events regardless and pull them into the cache: - self.__sock.setblocking(False) try: + self.__sock.setblocking(False) self.__json_read() except OSError as err: - if err.errno == errno.EAGAIN: - # No data available - pass - self.__sock.setblocking(True) + # EAGAIN: No data available; not critical + if err.errno != errno.EAGAIN: + raise + finally: + self.__sock.setblocking(True) # Wait for new events, if needed. # if wait is 0.0, this means "no wait" and is also implicitly false.