|
From: | John Snow |
Subject: | Re: [PULL 20/21] python/qemu/qmp.py: re-raise OSError when encountered |
Date: | Tue, 20 Oct 2020 15:06:45 -0400 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
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: + raiseIn 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.2Nir
[Prev in Thread] | Current Thread | [Next in Thread] |