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 <ldok...@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
>>
>
signature.asc
Description: OpenPGP digital signature
