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

[zhshzhou at linux.vnet.ibm.com]

From: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>

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 at linux.vnet.ibm.com>
---
 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
-- 
1.8.5.3