| Message ID | 20201006235817.3280413-19-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | python/qemu: strictly typed mypy conversion, pt2 | expand |
On 10/7/20 1:58 AM, John Snow 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> > --- > 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 > self.__sock.setblocking(True) > > # Wait for new events, if needed. > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
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: > + 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. The same kind of exception would raise QMPConnectError below instead of directly OSError. Should we try to make this consistent? > self.__sock.setblocking(True) > > # Wait for new events, if needed. Kevin
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: >> + 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
On 10/7/20 3:17 PM, John Snow wrote: > 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: >>> + 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. > Actually, wanna know a funny story? I think the reason this never broke anything is because sockfiles aren't suppose to be used in nonblocking mode -- their behavior is not documented in this case. In practice, what actually seems to happen when you set non-blocking mode and then call sockfile.readline() is that you get an empty string. This means you get 'None' from __json_read, and so on. Why doesn't this bite us more often? Well, we don't actually check the return value when we're using this in non-blocking mode, so I suppose that's the primary reason. I don't know if the behavior of Python changed at some point, I can try to patch this up for how Python *seems* to work, but we should probably do a more meaningful fix to get away from undefined behavior sometime soon. I had some async prototypes hanging around, maybe it's time to try and give that a more solid effort ... Related note: Even in blocking mode, you might get an empty reply from readline which means EOF and not just "try again." You'll see this in the case where you already have QEMU running from a previous failed test, and you start a new iotest up. When QMP calls accept(), the QMP capabilities handshake fails because it gets "None" from __json_read. Confusing error for what's actually going on there. It's actually that the socket is at EOF.
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 self.__sock.setblocking(True) # Wait for new events, if needed.
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(-)