[1/2] framework: handle UnicodeDecodeError

Submitted by Marek Olšák on Feb. 5, 2017, 7:28 p.m.

Details

Message ID 1486322922-15753-1-git-send-email-maraeo@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Marek Olšák Feb. 5, 2017, 7:28 p.m.
From: Marek Olšák <marek.olsak@amd.com>

this happened to me once
---
 framework/test/base.py | 2 ++
 1 file changed, 2 insertions(+)

Patch hide | download patch | download mbox

diff --git a/framework/test/base.py b/framework/test/base.py
index 4e7c8b2..756b08a 100644
--- a/framework/test/base.py
+++ b/framework/test/base.py
@@ -356,20 +356,22 @@  class Test(object):
                 os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
 
             # Since the process isn't running it's safe to get any remaining
             # stdout/stderr values out and store them.
             self.result.out, self.result.err = proc.communicate()
 
             raise TestRunError(
                 'Test run time exceeded timeout value ({} seconds)\n'.format(
                     self.timeout),
                 'timeout')
+        except UnicodeDecodeError as e:
+            raise TestRunError("UnicodeDecodeError.\n", 'crash')
 
         # The setter handles the bytes/unicode conversion
         self.result.out = out
         self.result.err = err
         self.result.returncode = returncode
 
     def __eq__(self, other):
         return self.command == other.command
 
     def __ne__(self, other):

Comments

On 05.02.2017 20:28, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> this happened to me once

Thanks, I vaguely remember seeing something like this as well. Both patches:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>

... though you might want to wait a bit to see if there are other 
opinions on the second one.

> ---
>  framework/test/base.py | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/framework/test/base.py b/framework/test/base.py
> index 4e7c8b2..756b08a 100644
> --- a/framework/test/base.py
> +++ b/framework/test/base.py
> @@ -356,20 +356,22 @@ class Test(object):
>                  os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
>
>              # Since the process isn't running it's safe to get any remaining
>              # stdout/stderr values out and store them.
>              self.result.out, self.result.err = proc.communicate()
>
>              raise TestRunError(
>                  'Test run time exceeded timeout value ({} seconds)\n'.format(
>                      self.timeout),
>                  'timeout')
> +        except UnicodeDecodeError as e:
> +            raise TestRunError("UnicodeDecodeError.\n", 'crash')
>
>          # The setter handles the bytes/unicode conversion
>          self.result.out = out
>          self.result.err = err
>          self.result.returncode = returncode
>
>      def __eq__(self, other):
>          return self.command == other.command
>
>      def __ne__(self, other):
>
I'm curious when you're hitting this because while this works around the problem
it's not really fixing it, and I'd really like to fix it correctly.

In the meantime I'm okay with this change:
Reviewed-by: Dylan Baker <dylan@pnwbakers.com>

Quoting Marek Olšák (2017-02-05 11:28:41)
> From: Marek Olšák <marek.olsak@amd.com>
> 
> this happened to me once
> ---
>  framework/test/base.py | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/framework/test/base.py b/framework/test/base.py
> index 4e7c8b2..756b08a 100644
> --- a/framework/test/base.py
> +++ b/framework/test/base.py
> @@ -356,20 +356,22 @@ class Test(object):
>                  os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
>  
>              # Since the process isn't running it's safe to get any remaining
>              # stdout/stderr values out and store them.
>              self.result.out, self.result.err = proc.communicate()
>  
>              raise TestRunError(
>                  'Test run time exceeded timeout value ({} seconds)\n'.format(
>                      self.timeout),
>                  'timeout')
> +        except UnicodeDecodeError as e:
> +            raise TestRunError("UnicodeDecodeError.\n", 'crash')
>  
>          # The setter handles the bytes/unicode conversion
>          self.result.out = out
>          self.result.err = err
>          self.result.returncode = returncode
>  
>      def __eq__(self, other):
>          return self.command == other.command
>  
>      def __ne__(self, other):
> -- 
> 2.7.4
> 
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit
On Mon, Feb 6, 2017 at 11:10 PM, Dylan Baker <dylan@pnwbakers.com> wrote:
> I'm curious when you're hitting this because while this works around the problem
> it's not really fixing it, and I'd really like to fix it correctly.

I don't remember. I guess the stdout/stderr output contained some
characters that python couldn't accept. For example, the LLVM
assembler prints errors to stdout/stderr with colors.

Marek
Quoting Marek Olšák (2017-02-06 14:39:03)
> On Mon, Feb 6, 2017 at 11:10 PM, Dylan Baker <dylan@pnwbakers.com> wrote:
> > I'm curious when you're hitting this because while this works around the problem
> > it's not really fixing it, and I'd really like to fix it correctly.
> 
> I don't remember. I guess the stdout/stderr output contained some
> characters that python couldn't accept. For example, the LLVM
> assembler prints errors to stdout/stderr with colors.
> 
> Marek

Interesting. Maybe the better thing to do then is to set the 'errors' flag of
bytes.decode to 'replace':

out.decode(encoding='utf-8', errors='replace')

Which would just take things it can't decode and use the � character. Does that
sound like a better plan since it would give a result other than crash?

Dylan
On Mon, Feb 6, 2017 at 11:57 PM, Dylan Baker <dylan@pnwbakers.com> wrote:
> Quoting Marek Olšák (2017-02-06 14:39:03)
>> On Mon, Feb 6, 2017 at 11:10 PM, Dylan Baker <dylan@pnwbakers.com> wrote:
>> > I'm curious when you're hitting this because while this works around the problem
>> > it's not really fixing it, and I'd really like to fix it correctly.
>>
>> I don't remember. I guess the stdout/stderr output contained some
>> characters that python couldn't accept. For example, the LLVM
>> assembler prints errors to stdout/stderr with colors.
>>
>> Marek
>
> Interesting. Maybe the better thing to do then is to set the 'errors' flag of
> bytes.decode to 'replace':
>
> out.decode(encoding='utf-8', errors='replace')
>
> Which would just take things it can't decode and use the � character. Does that
> sound like a better plan since it would give a result other than crash?

Any fix is OK with me, though I don't plan to spend any time on this.

Marek