diff mbox series

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

Message ID 20201006235817.3280413-19-jsnow@redhat.com (mailing list archive)
State New, archived
Headers show
Series python/qemu: strictly typed mypy conversion, pt2 | expand

Commit Message

John Snow Oct. 6, 2020, 11:58 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Oct. 7, 2020, 4:59 a.m. UTC | #1
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>
Kevin Wolf Oct. 7, 2020, 11:30 a.m. UTC | #2
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
John Snow Oct. 7, 2020, 7:17 p.m. UTC | #3
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
John Snow Oct. 8, 2020, 11:41 p.m. UTC | #4
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 mbox series

Patch

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.