|
From: | John Snow |
Subject: | Re: [PATCH 18/20] python/qemu/qmp.py: re-raise OSError when encountered |
Date: | Wed, 7 Oct 2020 15:17:10 -0400 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
On 10/7/20 7:30 AM, Kevin Wolf wrote:
Am 07.10.2020 um 01:58 hat John Snow geschrieben: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> --- python/qemu/qmp.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py index d911999da1f..bdbd1e9bdbb 100644 --- a/python/qemu/qmp.py +++ b/python/qemu/qmp.py @@ -169,9 +169,9 @@ def __get_events(self, wait: Union[bool, float] = False) -> None: try: self.__json_read() except OSError as err: - if err.errno == errno.EAGAIN: - # No data available - pass + # EAGAIN: No data available; not critical + if err.errno != errno.EAGAIN: + raiseHm, if we re-raise the exception here, the socket remains non-blocking. I think we intended to have it like that only temporarily.
Whoops. Yep, no good to go from one kind of broken to a different kind of broken.
The same kind of exception would raise QMPConnectError below instead of directly OSError. Should we try to make this consistent?
Yeah, honestly I'm a bit confused about the error plumbing myself. We like to return "None" a lot, and I have been trying to remove that whenever possible, because the nature of what None can mean semantically is ambiguous.
I need to sit down with this code and learn all of the different ways it can actually and genuinely fail, and what each failure actually semantically means.
I suspect it would probably be best to always catch socket errors and wrap them in QMPConnectError just to be consistent about that.
I also need to revise the docstrings to be clear about what errors get raised where, when, and why. I almost included that for this series, but decided against it because I need to also adjust the docstring formatting and so on -- and pending discussion in the qapi series -- felt it would be best to tackle that just a little bit later.
Here's a docstring convention question:I think that any method that directly raises an exception should always mention that with :raise X:. How far up the call chain, however, should anticipated exceptions be documented with :raise:?
My gut feeling is that it should stop at the first public call boundary, so accept() should repeat any :raise: comments that appear in private helpers.
self.__sock.setblocking(True)# Wait for new events, if needed.Kevin
Thanks for the review! Things seem like they're looking good. --js
[Prev in Thread] | Current Thread | [Next in Thread] |