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

[John Snow]

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