Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception

[Eduardo Habkost]

On Mon, Jul 24, 2017 at 02:13:09PM +0200, Lukáš Doktor wrote:
> Dne 21.7.2017 v 20:42 Eduardo Habkost napsal(a):
> > On Fri, Jul 21, 2017 at 08:37:34AM +0200, Lukáš Doktor wrote:
> >> Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a):
> >>> On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote:
> >>>> The naked Exception should not be widely used. It makes sense to be a
> >>>> bit more specific and use better-suited custom exceptions. As a benefit
> >>>> we can store the full reply in the exception in case someone needs it
> >>>> when catching the exception.
> >>>>
> >>>> Signed-off-by: Lukáš Doktor <address@hidden>
> >>>> ---
> >>>>  scripts/qemu.py | 17 +++++++++++++++--
> >>>>  1 file changed, 15 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/scripts/qemu.py b/scripts/qemu.py
> >>>> index 5948e19..2bd522f 100644
> >>>> --- a/scripts/qemu.py
> >>>> +++ b/scripts/qemu.py
> >>>> @@ -19,6 +19,19 @@ import subprocess
> >>>>  import qmp.qmp
> >>>>  
> >>>>  
> >>>> +class MonitorResponseError(qmp.qmp.QMPError):
> >>>> +    '''
> >>>> +    Represents erroneous QMP monitor reply
> >>>> +    '''
> >>>> +    def __init__(self, reply):
> >>>> +        try:
> >>>> +            desc = reply["error"]["desc"]
> >>>> +        except KeyError:
> >>>> +            desc = reply
> >>>> +        super(MonitorResponseError, self).__init__(desc)
> >>>> +        self.reply = reply
> >>>
> >>> This would require every user of self.reply to first check if
> >>> it's a string or dictionary.  All because of the "Monitor is
> >>> closed" case below:
> >>>
> >> I haven't used it for the `Monitor is closed` exception, so
> >> it's just to be able to store really broken reply safely.
> >> Anyway people can check whether the reply is a dict, or I can
> >> add `is_valid_reply` property which would check for
> >> `[error][desc]` presence (which is a bit more precise than just
> >> plain dict type checking).
> > 
> > 
> > Oops, I wasn't paying enough attention, sorry.  self.reply is
> > actually always set to the response from the monitor.
> > 
> > If you are just trying a safe fallback for 'desc' if the response
> > broken, what about using repr(reply) or json.dumps(reply) if
> > reply['error']['desc'] isn't set?
> > 
> I could use repr, but I'd prefer keeping it the way it is as
> you could use `isinstance` to see whether it's dict and
> interact with it (if needed, eg. on negative testing, which is
> my motivation for storing the full response).

This makes sense for self.reply, but I'm thinking about the
argument to Exception.__init__().  I'm worried that things might
break if the argument is not a string.

-- 
Eduardo