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

[Lukáš Doktor]

Dne 24.7.2017 v 17:32 Eduardo Habkost napsal(a):
> On Mon, Jul 24, 2017 at 02:13:09PM +0200, Lukáš Doktor wrote:
>> 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).
> 
> This makes sense for self.reply, but I'm thinking about the
> argument to Exception.__init__().  I'm worried that things might
> break if the argument is not a string.
> 

I see. It's python so it'll simply use `__str__` method, but you are right that 
for the exception description the use of `repr` is better. I'll change it in v2 
(keeping the `self.reply` as is).
Lukáš

signature.asc
Description: OpenPGP digital signature