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

From: John Snow <jsnow@redhat.com>
To: Nir Soffer <nsoffer@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	qemu-block <qemu-block@nongnu.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Max Reitz" <mreitz@redhat.com>, "Cleber Rosa" <crosa@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PULL 20/21] python/qemu/qmp.py: re-raise OSError when encountered
Date: Tue, 20 Oct 2020 15:06:45 -0400	[thread overview]
Message-ID: <3a096ef6-36a3-73ac-e0e8-90a3c4473b5d@redhat.com> (raw)
In-Reply-To: <CAMRbyyuiqFK2biX5ADrx0cDJ1V-naiOHJ7TKD1M-rGFnFH6c4g@mail.gmail.com>

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
> 