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

[Lukáš Doktor]

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

Lukáš

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

signature.asc
Description: OpenPGP digital signature