[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather tha
From: |
Lukáš Doktor |
Subject: |
Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception |
Date: |
Fri, 21 Jul 2017 08:37:34 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
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).
>> +
>> +
>> 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
- Re: [Qemu-devel] [PATCH 03/11] qemu.py: Use iteritems rather than keys(), (continued)
[Qemu-devel] [PATCH 09/11] qmp.py: Avoid overriding a builtin object, Lukáš Doktor, 2017/07/20
[Qemu-devel] [PATCH 10/11] qtest.py: Few pylint/style fixes, Lukáš Doktor, 2017/07/20