[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PULL 20/21] python/qemu/qmp.py: re-raise OSError when encountered

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)
+            self.__sock.setblocking(False)

This change is not required. The idiom is:

      do stuff
          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.

          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:

    except 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.


+        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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]