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

["Lukáš Doktor" <ldoktor@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3401 bytes --]

Dne 25.7.2017 v 18:06 Eduardo Habkost napsal(a):
> 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 <ldoktor@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>  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))
> 
Well I know I can implement custom `__str__` class, but the default one simply stores the argument as string and reports it, so for simple strings I usually go with super call and with complex ones with custom `__str__`. Anyway if this suits this project style better I'll change it in v2.

Lukáš

> 
> [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
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 502 bytes --]