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

[Eduardo Habkost]

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?

> 
> >> +
> >> +
> >>  class QEMUMachine(object):
> >>      '''A QEMU VM'''
> >>  
> >> @@ -197,9 +210,9 @@ class QEMUMachine(object):
> >>          '''
> >>          reply = self.qmp(cmd, conv_keys, **args)
> >>          if reply is None:
> >> -            raise Exception("Monitor is closed")
> >> +            raise qmp.qmp.QMPError("Monitor is closed")
> > 
> > Should we really use the same exception type for this, if it's
> > not really a QMP monitor error reply, and won't even have a real
> > QMP reply in self.reply?
> > 
> I wasn't sure but the same exception can be caused by other
> failures when obtaining replies so I think in case the monitor
> is closed we might expect the same exception. Would importing
> it in the top level of this module to become `qemu.QMPError`
> work for you better, or would you prefer IOError, RuntimeError
> or another custom exception?

I was not paying enough attention.  QMPError sounds good to me.

Reviewed-by: Eduardo Habkost <address@hidden>

> 
> Lukáš
> 
> > 
> >>          if "error" in reply:
> >> -            raise Exception(reply["error"]["desc"])
> >> +            raise MonitorResponseError(reply)
> >>          return reply["return"]
> >>  
> >>      def get_qmp_event(self, wait=False):
> >> -- 
> >> 2.9.4
> >>
> > 
> 
> 




-- 
Eduardo