[PATCH 0/3] fdtdump: Make output prettier

[PATCH 0/3] fdtdump: Make output prettier

[Pantelis Antoniou]

fdtdump output can be very hard to read;

This patch makes sure that escaped characters are correctly detected
as strings and that output respects a given column width.

Pantelis Antoniou (3):
  util: Add method for escape output handling
  fdtdump: Prettify output of properties
  manual: Document prettification fdtdump options

 Documentation/manual.txt |   2 +
 fdtdump.c                | 103 ++++++++++++++++++++++++++++++++++++++++++++---
 util.c                   |  70 +++++++++++++++++++++++++++++++-
 util.h                   |  35 ++++++++++++++++
 4 files changed, 204 insertions(+), 6 deletions(-)

-- 
2.1.4

[PATCH 1/3] util: Add method for escape output handling

[Pantelis Antoniou]

Add 3 methods that help when dealing with escaping output strings.

util_isesc() checks whether a character should be escaped.
util_c2str() converts a character to a possibly escaped sequence
util_quoted_strlen() calculates the length of a quoted string.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 util.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 util.h | 35 +++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/util.c b/util.c
index 9953c32..6895515 100644
--- a/util.c
+++ b/util.c
@@ -100,10 +100,17 @@ char *join_path(const char *path, const char *name)
 	return str;
 }
 
+bool util_isesc(char c)
+{
+	return c == '\a' || c == '\b' || c == '\t' || c == '\n' || c == '\v' ||
+	       c == '\f' || c == '\r' || c == '\\' || c == '\"';
+}
+
 bool util_is_printable_string(const void *data, int len)
 {
 	const char *s = data;
 	const char *ss, *se;
+	char c;
 
 	/* zero length is not */
 	if (len == 0)
@@ -117,8 +124,13 @@ bool util_is_printable_string(const void *data, int len)
 
 	while (s < se) {
 		ss = s;
-		while (s < se && *s && isprint((unsigned char)*s))
+		while (s < se && *s) {
+			c = *s;
+			if (!(isprint(c) || util_isesc(c)))
+				break;
 			s++;
+		}
+
 
 		/* not zero, or not done yet */
 		if (*s != '\0' || s == ss)
@@ -130,6 +142,62 @@ bool util_is_printable_string(const void *data, int len)
 	return 1;
 }
 
+char *util_c2str(char c, char *buf, int bufsz)
+{
+	char *s = buf;
+
+	if (util_isesc(c)) {
+
+		if (bufsz < 3)
+			return NULL;
+
+		/* escape case */
+		*s++ = '\\';
+		switch (c) {
+		case '\a': *s++ =  'a'; break;
+		case '\b': *s++ =  'b'; break;
+		case '\t': *s++ =  't'; break;
+		case '\n': *s++ =  'n'; break;
+		case '\v': *s++ =  'v'; break;
+		case '\f': *s++ =  'f'; break;
+		case '\r': *s++ =  'r'; break;
+		case '\\': *s++ = '\\'; break;
+		case '\"': *s++ = '\"'; break;
+		}
+	} else if (!isprint(c)) {
+		static const char *hexb = "0123456789abcdef";
+
+		if (bufsz < 5)
+			return NULL;
+
+		/* hexadecimal escape case */
+		*s++ = '\\';
+		*s++ = 'x';
+		*s++ = hexb[(((unsigned int)c >> 4) & 0xf)];
+		*s++ = hexb[  (unsigned int)c       & 0xf ];
+	} else {
+		if (bufsz < 2)
+			return NULL;
+
+		*s++ = c;	/* normal printable */
+	}
+
+	*s = '\0';
+
+	return buf;
+}
+
+int util_quoted_strlen(const char *str)
+{
+	int len;
+	char c, buf[C2STR_BUF_MAX];
+
+	len = 1;
+	while ((c = *str++) != '\0' && util_c2str(c, buf, sizeof(buf)))
+		len += strlen(buf);
+	return len + 1;
+}
+
 /*
  * Parse a octal encoded character starting at index i in string s.  The
  * resulting character will be returned and the index i will be updated to
diff --git a/util.h b/util.h
index ad5f411..3ed617c 100644
--- a/util.h
+++ b/util.h
@@ -82,6 +82,41 @@ extern char *join_path(const char *path, const char *name);
  */
 bool util_is_printable_string(const void *data, int len);
 
+/**
+ * Check if this is is a character we should escape.
+ * The list of valid escaped chars is: \a\b\t\n\b\f\r\\\"
+ *
+ * @param c	The character to check
+ * @return 1 if the character is one that should be escaped
+ */
+bool util_isesc(char c);
+
+/**
+ * Convert a given character to it's escaped form
+ * If it's a normal printable character the buffer is filled with "c\0"
+ * If it's an escaped character the corresponding escape * is terminated
+ * with \<esc>\0
+ * For any other a hex form is used (\xYY) where YY is the ascii in hex of c.
+ *
+ * @param c	The character to convert
+ * @param buf	The corresponding buffer to fill
+ * @param bufsz The maximum buffer size (including NULL).
+ * 		Note that the absolute maximum buffer fill is 5 for \xYY\0
+ * @return 	buf if the result fits or NULL on error
+ */
+char *util_c2str(char c, char *buf, int bufsz);
+
+/* maximum buffer for c2str */
+#define C2STR_BUF_MAX	5
+
+/**
+ * Return the length in characters when quoting the given string
+ *
+ * @param str	The string to query it's quoted length
+ * @return	Length in characters of the quoted string generated from str.
+ */
+int util_quoted_strlen(const char *str);
+
 /*
  * Parse an escaped character starting at index i in string s.  The resulting
  * character will be returned and the index i will be updated to point at the
-- 
2.1.4

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

[Pantelis Antoniou]

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.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 fdtdump.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 98 insertions(+), 5 deletions(-)

diff --git a/fdtdump.c b/fdtdump.c
index fa3b561..82aec9a 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -22,6 +22,9 @@
 #define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
 #define GET_CELL(p)	(p += 4, *((const fdt32_t *)(p-4)))
 
+static int width = 80;
+static int shift = 4;
+
 static const char *tagname(uint32_t tag)
 {
 	static const char * const names[] = {
@@ -42,6 +45,86 @@ static const char *tagname(uint32_t tag)
 #define dumpf(fmt, args...) \
 	do { if (debug) printf("// " fmt, ## args); } while (0)
 
+static void print_prop(int col, const char *data, int len)
+{
+	int i, j, qlen, span, fit;
+	const char *s;
+	char c, buf[C2STR_BUF_MAX];
+
+	/* no data, don't print */
+	if (len == 0)
+		return;
+
+	printf(" = ");
+	col += strlen(" = ");
+
+	if (util_is_printable_string(data, len)) {
+		s = data;
+		j = col;
+		do {
+			qlen = util_quoted_strlen(s);
+			if (s > data) {
+				printf(",");
+				j++;
+
+				if (j + qlen > width - 1) {
+					j = col;
+					printf("\n%*s", col, "");
+				} else {
+					printf(" ");
+					j++;
+				}
+			}
+
+			/* print quoted string */
+			putchar('\"');
+			while ((c = *s++) != '\0' && util_c2str(c, buf, sizeof(buf)))
+				printf("%s", buf);
+			putchar('\"');
+
+			j += qlen;
+		} while (s < data + len);
+
+	} else if ((len % 4) == 0) {
+		const fdt32_t *cell = (const fdt32_t *)data;
+
+		len /= 4;
+
+		/* how many words can we fit? */
+		fit = (width - 1 - col - 2) / 11;
+
+		do {
+			printf("<");
+			span = len > fit ? fit : len;
+			for (i = 0; i < span; i++)
+				printf("0x%08x%s", fdt32_to_cpu(cell[i]),
+						i < (span - 1) ? " " : "");
+			cell += span;
+			len -= span;
+			printf(">");
+			if (len > 0)
+				printf(",\n%*s", col, "");
+		} while (len > 0);
+	} else {
+		const unsigned char *p = (const unsigned char *)data;
+
+		/* how many bytes can we fit? */
+		fit = (width - 1 - col - 2) / 3;
+
+		do {
+			printf("[");
+			span = len > fit ? fit : len;
+			for (i = 0; i < span; i++)
+				printf("%02x%s", *p++,
+						i < span - 1 ? " " : "");
+			len -= span;
+			printf("]");
+			if (len > 0)
+				printf(",\n%*s", col, "");
+		} while (len > 0);
+	}
+}
+
 static void dump_blob(void *blob, bool debug)
 {
 	uintptr_t blob_off = (uintptr_t)blob;
@@ -57,12 +140,11 @@ static void dump_blob(void *blob, bool debug)
 	uint32_t totalsize = fdt32_to_cpu(bph->totalsize);
 	uint32_t tag;
 	const char *p, *s, *t;
-	int depth, sz, shift;
+	int depth, sz;
 	int i;
 	uint64_t addr, size;
 
 	depth = 0;
-	shift = 4;
 
 	printf("/dts-v1/;\n");
 	printf("// magic:\t\t0x%x\n", fdt32_to_cpu(bph->magic));
@@ -127,7 +209,8 @@ static void dump_blob(void *blob, bool debug)
 		}
 
 		if (tag != FDT_PROP) {
-			fprintf(stderr, "%*s ** Unknown tag 0x%08x\n", depth * shift, "", tag);
+			fprintf(stderr, "%*s ** Unknown tag 0x%08x\n",
+					depth * shift, "", tag);
 			break;
 		}
 		sz = fdt32_to_cpu(GET_CELL(p));
@@ -141,22 +224,26 @@ static void dump_blob(void *blob, bool debug)
 		dumpf("%04zx: string: %s\n", (uintptr_t)s - blob_off, s);
 		dumpf("%04zx: value\n", (uintptr_t)t - blob_off);
 		printf("%*s%s", depth * shift, "", s);
-		utilfdt_print_data(t, sz);
+		print_prop(depth * shift + strlen(s), t, sz);
 		printf(";\n");
 	}
 }
 
 /* Usage related data. */
 static const char usage_synopsis[] = "fdtdump [options] <file>";
-static const char usage_short_opts[] = "ds" USAGE_COMMON_SHORT_OPTS;
+static const char usage_short_opts[] = "dsw:S:" USAGE_COMMON_SHORT_OPTS;
 static struct option const usage_long_opts[] = {
 	{"debug",            no_argument, NULL, 'd'},
 	{"scan",             no_argument, NULL, 's'},
+	{"width",	     required_argument, NULL, 'w' },
+	{"shift",	     required_argument, NULL, 'S' },
 	USAGE_COMMON_LONG_OPTS
 };
 static const char * const usage_opts_help[] = {
 	"Dump debug information while decoding the file",
 	"Scan for an embedded fdt in file",
+	"Width of output",
+	"Shift space amount when recursing",
 	USAGE_COMMON_OPTS_HELP
 };
 
@@ -198,6 +285,12 @@ int main(int argc, char *argv[])
 		case 's':
 			scan = true;
 			break;
+		case 'w':
+			width = atoi(optarg);
+			break;
+		case 'S':
+			shift = atoi(optarg);
+			break;
 		}
 	}
 	if (optind != argc - 1)
-- 
2.1.4

[PATCH 3/3] manual: Document prettification fdtdump options

[Pantelis Antoniou]

Document the new fdtdump options that make output prettier.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 Documentation/manual.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/manual.txt b/Documentation/manual.txt
index 72403ac..27ca3af 100644
--- a/Documentation/manual.txt
+++ b/Documentation/manual.txt
@@ -679,6 +679,8 @@ The syntax of the fdtdump command line is:
 Where options are:
     -d,--debug          Dump debug information while decoding the file
     -s,--scan           Scan for an embedded fdt in given file
+    -w,--width		Width of output in columns
+    -S,--shift		Shift space amount when recursing in a node
 
 3) fdtoverlay -- Flat Device Tree overlay applicator
 
-- 
2.1.4

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

[David Gibson]

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

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.

-- 
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 --]

Re: [PATCH 0/3] fdtdump: Make output prettier

[David Gibson]

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

On Wed, Jun 14, 2017 at 05:53:47PM +0300, Pantelis Antoniou wrote:
> fdtdump output can be very hard to read;
> 
> This patch makes sure that escaped characters are correctly detected
> as strings and that output respects a given column width.

I really wish people would stop trying to use fdtdump as something
it's not.  It's a trivial debug tool, nothing more.  Options for
pretty don't belong here.  If you want nicer dtb->dts output, use dtc
-I dtb -O dts.

> 
> Pantelis Antoniou (3):
>   util: Add method for escape output handling
>   fdtdump: Prettify output of properties
>   manual: Document prettification fdtdump options
> 
>  Documentation/manual.txt |   2 +
>  fdtdump.c                | 103 ++++++++++++++++++++++++++++++++++++++++++++---
>  util.c                   |  70 +++++++++++++++++++++++++++++++-
>  util.h                   |  35 ++++++++++++++++
>  4 files changed, 204 insertions(+), 6 deletions(-)
> 

-- 
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 --]

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

[Pantelis Antoniou]

Hi David,

On Wed, 2017-06-14 at 23:06 +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.
> 

I'm afraid it's not so clear cut.

True, you can use dtc on the host to dump a blob to DTS in a similar
manner to fdtdump (with the prettified output is much better with this
patch).

Although the size of the dtc compiler package is larger than fdtdump by
about 5 times (this is not that important nowdays), the biggest use is
for restricted environment where the full dtc compiler just can't work.

A bootloader or something small can easily incorporate fdtdump for
dumping out blobs, while including dtc is not even be possible.

Anyway, the purpose of this patch is to make an already existing tool
better. If you feel that strongly about it duplicating functionality you
should just remove it and leave it at that.

Regards

-- Pantelis

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

[Tom Rini]

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.

Swinging back around to what Pantelis is talking about, it would be kind
of annoying, but we could probably integrate dtc into U-Boot like the
kernel has.  Is that where we're at then?  Thanks!

-- 
Tom

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

[Frank Rowand]

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?

Thanks,

Frank

> Swinging back around to what Pantelis is talking about, it would be kind
> of annoying, but we could probably integrate dtc into U-Boot like the
> kernel has.  Is that where we're at then?  Thanks!
> 

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

[Tom Rini]

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

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.

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?

-- 
Tom

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

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

[Frank Rowand]

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'.

The maintainer wants to keep the debug tool simple.  Another tool (dtc)
already exists to do what the proposed patch would add.

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?


> 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.

-Frank



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tom Rini]

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

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).

> 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.

> > 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.

-- 
Tom

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

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

[David Gibson]

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

On Wed, Jun 14, 2017 at 10:12:54PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> On Wed, 2017-06-14 at 23:06 +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.
> > 
> 
> I'm afraid it's not so clear cut.
> 
> True, you can use dtc on the host to dump a blob to DTS in a similar
> manner to fdtdump (with the prettified output is much better with this
> patch).
> 
> Although the size of the dtc compiler package is larger than fdtdump by
> about 5 times (this is not that important nowdays), the biggest use is
> for restricted environment where the full dtc compiler just can't work.
> 
> A bootloader or something small can easily incorporate fdtdump for
> dumping out blobs, while including dtc is not even be possible.

Ok, but unlike libfdt, fdtdump will need at least minimal modification
to work in that restricted environment.  And if you're going to modify
it that way, you can add whatever pretty printing options you want.

-- 
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 --]

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

[David Gibson]

[-- 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 --]