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 <[email protected]> >>>> --- >>>> 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 <[email protected]>
>
>>
>> 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
