Re: [PATCH 2/3] fdtdump: Prettify output of properties

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5344 bytes --]

On Fri, Jun 16, 2017 at 08:34:49PM -0400, Tom Rini wrote:
> On Fri, Jun 16, 2017 at 12:01:35PM -0700, Frank Rowand wrote:
> > On 06/16/17 08:40, Tom Rini wrote:
> > > On Thu, Jun 15, 2017 at 06:17:20PM -0700, Frank Rowand wrote:
> > >> Hi Tom,
> > >>
> > >> On 06/15/17 16:52, Tom Rini wrote:
> > >>> On Wed, Jun 14, 2017 at 11:06:39PM +0800, David Gibson wrote:
> > >>>> On Wed, Jun 14, 2017 at 05:53:49PM +0300, Pantelis Antoniou wrote:
> > >>>>> Dumping files with large properties results in output with
> > >>>>> arbitrary long lines.
> > >>>>>
> > >>>>> Original (manual line breaks inserted; it's a single long line):
> > >>>>>
> > >>>>> / {
> > >>>>>     int = <0x00000001 0x00000024 0x00000004 0x00000000 \
> > >>>>> 0x000502a4 0x000000df 0x00000003 0x13885783 0x13885783 \
> > >>>>> 0x00000002 0x62797465 0x00000000 0x00000000 0x00000000 \
> > >>>>> 0x00000000 0x00000000 0x00000000 0x00000000 0x00000000 \
> > >>>>> 0x00000000 0x00000000 0x00000000 0x00000000 0x00000000>;
> > >>>>> };
> > >>>>>
> > >>>>> After prettification:
> > >>>>>
> > >>>>> / {
> > >>>>>     int = <0x00000001 0x00000002 0x00000008 0x00000010 0x00000024 0x000000ab>,
> > >>>>>           <0x00000001 0x00000017 0x00000004 0x00000038 0x00000007 0x00000009>,
> > >>>>>           <0x00000000 0x00000068 0x00000214 0x0000b8d9 0x000502a4 0x00000001>,
> > >>>>>           <0x00000004 0x0000002b 0x000000df 0x00000003 0x00000002 0x00000001>;
> > >>>>> };
> > >>>>>
> > >>>>> There are two new options (-w/--width) and (-S/--shift).
> > >>>>>
> > >>>>> Width is the terminal width, shift is the amount of spaces each nest level
> > >>>>> increases by.
> > >>>>>
> > >>>>> Width by default is set to 80, and shift to 4.
> > >>>>
> > >>>> Nack.
> > >>>>
> > >>>> fdtdump is supposed to be a trivial debug tool.   If you want to
> > >>>> decompile dtbs "for real" use dtc -I dtb -O dts.
> > >>>
> > >>> There's been times, entirely unrelated to what Pantelis is doing, where
> > >>> I've used fdtdump in production cases because I needed to whack a few
> > >>> things around.  If it's just supposed to be a trivial debug tool, we've
> > >>> likely moved well beyond the point where we need to keep trivial tools
> > >>> around if they shouldn't be more widely used, IMHO.
> > >>
> > >> Let me paraphrase what I think that said:
> > >>
> > >>    If a trivial debug tool is used by a wide audience then we should get
> > >>    rid of the tool.
> > >>
> > >> I suspect I misunderstood.  Can you clarify?
> > > 
> > > Sure.  Pantelis wants to improve a trivial debug tool to be slightly
> > > more useful.  The maintainer says no, we shouldn't touch the tool, you
> > > can use dtc -I dtb -O dts instead.  As that would also cover fdtdump
> > > itself, it sounds like fdtdump is deprecated and should be removed, as
> > > it's being used outside of the trivial debug use case.
> > 
> > fdtdump is useful for debugging and provides several features that aren't
> > available in 'dtc -I dtb -O dts'.
> 
> Agreed.
> 
> > The maintainer wants to keep the debug tool simple.
> 
> Maintainers prerogative, yes.
> 
> > Another tool (dtc)
> > already exists to do what the proposed patch would add.
> 
> I disagree, or at least don't see it in 'dtc -I dtb -O dts' as there
> would need to be another argument for "linebreak the output and continue
> to be valid".  I just rebuilt from master and dumped a dtb I had around
> to confirm (and checked the help, too).

No, I don't think it's there.  But I'd be happy to accept patches
adding pretty printing to the -O dts output.  I particularly don't
want to add convenience features to fdtdump that _arent't_ already in
dtc.

> > Somehow Pantelis and you then jumped to the conclusion that fdtdump
> > should be removed.  This is the part that I don't get.  It is a useful
> > tool that has features that are not otherwise available.  Why would
> > you get rid of it?
> 
> It's not Pantelis, it's my argument.  And for the record, I say it's
> useful too.  I'm arguing that the logical end point of the maintainers
> argument is that fdtdump shouldn't be used anywhere by anyone, it's a
> trivial debug tool that no one should be shipping.  It's like enabling
> various debug options in the kernel.  The right tool for most people of
> "I need to read a dtb" is to use dtc -I dtb -O dts, and if you're
> working on dtc, that's when maybe you need another tool at times, so you
> can see the internal steps.

The main use case for fdtdump, as far as I'm concerned, is if you have
a (possibly) corrupted dtb.  dtc will die without printing anything in
that case, fdtdump will probably give you at least something.

> 
> > > Or, can we talk about improving fdtdump as being a valid tool working
> > > with dtb files when 'dtc -I dtb -O dts' is not desired?
> > 
> > That conflicts with the purpose (debug) and design goals (trivial)
> > stated by the maintainer.
> 
> So what's the right tool for getting nice human readable output?  'dtc
> -I dtb -O dts' will give you the arbitrarily long lines that this patch
> fixes.
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]