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

[Aline Manera]

Reviewed-by: Aline Manera <alinefm at linux.vnet.ibm.com>

On 04/03/2014 01:01 AM, zhshzhou at linux.vnet.ibm.com wrote:
> 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