Message-ID: <1498238944.32344.34.camel@nxp.com>
Subject: Re: [PATCH 2/2] scripts/gdb: lx-dmesg: Use errors=replace for
 decoding
From: Leonard Crestez <leonard.crestez@nxp.com>
To: Jan Kiszka <jan.kiszka@siemens.com>
CC: Kieran Bingham <kieran@ksquared.org.uk>,
        <linux-kernel@vger.kernel.org>
Date: Fri, 23 Jun 2017 20:29:04 +0300
In-Reply-To: <cb6751a5-4a14-01b9-785f-48bfa67d2c89@siemens.com>
References: <00d6621945ececb6c9fb360aef0efd5747439a38.1498227531.git.leonard.crestez@nxp.com>
         <bd645eb6b3f3c2e0a4d01e6802eaf19069d25d33.1498227531.git.leonard.crestez@nxp.com>
         <cb6751a5-4a14-01b9-785f-48bfa67d2c89@siemens.com>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jun 2017 17:29:07.3121
 (UTC)
X-MS-Exchange-CrossTenant-Id: 5afe0b00-7697-4969-b663-5eab37d5f47e
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=5afe0b00-7697-4969-b663-5eab37d5f47e;Ip=[192.88.168.50];Helo=[tx30smr01.am.freescale.net]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR0301MB0907
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2793
Lines: 67

On Fri, 2017-06-23 at 18:02 +0200, Jan Kiszka wrote:
> On 2017-06-23 16:20, Leonard Crestez wrote:
> > 
> > It is never desirable lx-dmesg to fail on string decoding errors,
> > not
> > even if the log buffer is corrupt.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > ---
> >  scripts/gdb/linux/dmesg.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/scripts/gdb/linux/dmesg.py
> > b/scripts/gdb/linux/dmesg.py
> > index 6f8d2b2..d0cac58 100644
> > --- a/scripts/gdb/linux/dmesg.py
> > +++ b/scripts/gdb/linux/dmesg.py
> > @@ -52,13 +52,13 @@ class LxDmesg(gdb.Command):
> >                  continue
> >  
> >              text_len = utils.read_u16(log_buf[pos + 10:pos + 12])
> > -            text = log_buf[pos + 16:pos + 16 + text_len].decode()
> > +            text = log_buf[pos + 16:pos + 16 +
> > text_len].decode(errors='replace')
> pep8 should complain.
> 
> > 
> >              time_stamp = utils.read_u64(log_buf[pos:pos + 8])
> >  
> >              for line in text.splitlines():
> >                  gdb.write("[{time:12.6f}] {line}\n".format(
> >                      time=time_stamp / 1000000000.0,
> > -                    line=line))
> > +                    line=line.encode(errors='replace')))
> You only talk about "decoding" in the commit log, but here you encode
> back. An short explanation why this is also needed would be nice.
> 
Apparently .decode(errors='replace') will return an unicode string
where invalid characters are replaced with U+FFFD REPLACEMENT
CHARACTER. Attempting to encode that back to the default ascii encoding
of python2 throws an error, using errors='replace' results in a '?'
instead.

See: https://docs.python.org/2/library/codecs.html#codec-base-classes

In python3 the default encoding seems to be utf8 and errors='replace'
is not obviously required on the encode step.

I don't actually have a gdb version compiled with python3 support and
don't know if gdb.write always properly handles unicode in all cases.
Perhaps it might be better to also explicitly specify 'utf8' as the
encoding?

Linux does occasionally print unicode, for example the jffs2 driver
shows an copyright symbol at startup. Using errors='replace' everywhere
on python2 results in this output from lx-dmesg:

[    0.367578] jffs2: version 2.2. (NAND) ?? 2001-2006 Red Hat, Inc.

In theory if we use decode('utf8', errors='replace') and encode('utf8')
then errors='replace' would not be required on the encode side.
Honestly for debug code it might be preferable to do the safest
possible thing and go 'ascii' everywhere.

--
Regards,
Leonard