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 <js...@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:
+ raise
Hm, 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