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