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

[John Snow]

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:
> > +                raise
>
> In 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.2
>
> Nir