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

[Eduardo Habkost]

On Tue, Jul 25, 2017 at 05:09:46PM +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>
> Reviewed-by: Eduardo Habkost <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..e6df54c 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__(repr(desc))

This will end up calling Exception.__init__.  I previously
suggested repr(desc) above(*) because I didn't know what happened
when the Exception.__init__ argument is not a string.

I couldn't find any documentation on the right argument types for
Exception.__init__.  The examples in the Python documentation[1]
don't call Exception.__init__ at all: they simply implement
__str__().

However, based on my testing and on the BaseException
documentation[2], I believe repr() isn't really necessary:

"If str() or unicode() is called on an instance of this class,
the representation of the argument(s) to the instance are
returned, or the empty string when there were no arguments."

So your previous version was good, already.

But maybe we could do this instead, to make the constructor as
simple as possible:

class MonitorResponseError(qmp.qmp.QMPError):
    def __init__(self, reply):
        self.reply = reply

    def __str__(self):
        return self.reply.get('error', {}).get('desc', repr(self.reply))


[1] https://docs.python.org/2/tutorial/errors.html#user-defined-exceptions
[2] https://docs.python.org/2/library/exceptions.html#exceptions.BaseException

> +        self.reply = reply
> +
> +
>  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")
>          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