Re: [Kimchi-devel] [PATCH v2] kimchi.exception: Properly Decode All Kinds of Exception Arguments

From: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; KimchiException provides the ability to translate each argument and the error message. It decodes the translated message to unicode. It's also smart and avoids decoding a unicode argument twice by a check to make sure the to-be-decoded value is not unicode. The problem is that when a KimchiException is initialized by an object that can format itself to a unicode sting, for example, try: ... except OperationFailed as e: ... raise OperationFailed('KCHXXXX', {'err': e}) the variable "e" is not a unicode object so it passes the check mentioned above, then it gets decoded twice and raises an encoding exception. This patch does not only rely on the type of the to-be-decoded object, but also catches the encoding exeption. It firstly tries to call str(obj) to get the ascii string of the obj, then decodes it to unicode. This is valid for a normal string or any object that formats itself to a normal string. If it fails, it tries to call unicode(obj) to get the unicode string of the object. This is valid for objects that formats themselves to a unicode string. In all, it handles various objects like following. If obj is a unicode string, use it directly. If obj is a normal string, decode it to a unicode string. If obj can format itself to a normal string, format it and decode it to unicode. If obj can format itself to a unicode string, format it. Another problem is that by the current design, KimchiException always stores error message in unicode. This can cause encoding exception when we call "str(KimchiExceptionObject)". This patch is not a total refactor of the translation and encoding so it does not solve this problem. The workaround is using KimchiExceptionObject.message instead of str it. Notice: I'm following the Python str.encode and decode semantics in the commit message. "encode" means converting unicode sting to ascii/utf-8. "decode" means converting ascii/utf-8 string to unicode. "encoding" means the action of decode and encode in general. v2 sosreport_generate() function in debugreport code encapsules an OperationFailed in another OperationFailed and re-raise. This leads to odd error message like follow: KCHDR0005E: Unable to generate debug report my-debugreport. Details: KCHDR0003E: Unable to create debug report my-debugreport. Details: -9. The v2 patch excepts KimchiException and re-raise it directly to produce a clean error message. For the rest vast kinds of exceptions, it still translates it to OperationFailed and encapsules it. As a result, all error recognized by Kimchi produces a clean error message with specific KCHXXXX error code. And unknown error produces an error message with KCHDR0005E error code following by the detailed exception. Though the change in v2 solves a root cause and does not make use of the KimchiException decoding improvements, the author still keeps the decoding code to make KimchiException more robust to all kinds of parameters in general. Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; --- src/kimchi/asynctask.py | 2 +- src/kimchi/exception.py | 8 +++++++- src/kimchi/model/debugreports.py | 16 ++++++++++++---- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/kimchi/asynctask.py b/src/kimchi/asynctask.py index b54f143..9805f33 100644 --- a/src/kimchi/asynctask.py +++ b/src/kimchi/asynctask.py @@ -70,4 +70,4 @@ class AsyncTask(object): except Exception, e: cherrypy.log.error_log.error("Error in async_task %s " % self.id) cherrypy.log.error_log.error(traceback.format_exc()) - cb("Unexpected exception: %s" % str(e), False) + cb("Unexpected exception: %s" % e.message, False) diff --git a/src/kimchi/exception.py b/src/kimchi/exception.py index 87982ea..fcf60cc 100644 --- a/src/kimchi/exception.py +++ b/src/kimchi/exception.py @@ -54,7 +54,13 @@ class KimchiException(Exception): for key, value in args.iteritems(): if not isinstance(value, unicode): - args[key] = unicode(str(value), 'utf-8') + try: + # In case the value formats itself to an ascii string. + args[key] = unicode(str(value), 'utf-8') + except UnicodeEncodeError: + # In case the value is a KimchiException or it formats + # itself to a unicode string. + args[key] = unicode(value) return unicode(translation.gettext(text), 'utf-8') % args diff --git a/src/kimchi/model/debugreports.py b/src/kimchi/model/debugreports.py index 66e4ef5..cd31b31 100644 --- a/src/kimchi/model/debugreports.py +++ b/src/kimchi/model/debugreports.py @@ -26,7 +26,7 @@ import subprocess import time from kimchi import config -from kimchi.exception import NotFoundError, OperationFailed +from kimchi.exception import KimchiException, NotFoundError, OperationFailed from kimchi.model.tasks import TaskModel from kimchi.utils import add_task, kimchi_log from kimchi.utils import run_command @@ -64,6 +64,10 @@ class DebugReportsModel(object): @staticmethod def sosreport_generate(cb, name): + def log_error(e): + log = logging.getLogger('Model') + log.warning('Exception in generating debug file: %s', e) + try: command = ['sosreport', '--batch', '--name=%s' % name] output, error, retcode = run_command(command) @@ -109,15 +113,19 @@ class DebugReportsModel(object): cb('OK', True) return - except OSError: + except KimchiException as e: + log_error(e) + raise + + except OSError as e: + log_error(e) raise except Exception, e: # No need to call cb to update the task status here. # The task object will catch the exception raised here # and update the task status there - log = logging.getLogger('Model') - log.warning('Exception in generating debug file: %s', e) + log_error(e) raise OperationFailed("KCHDR0005E", {'name': name, 'err': e}) @staticmethod