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

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

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

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 <ldoktor@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..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).

>> +
>> +
>>  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?

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



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