[PATCH 1/1] fdtdump.c: provide colored output

[PATCH 1/1] fdtdump.c: provide colored output

[Heinrich Schuchardt]

A new command line option is added
  -c, --color <arg> Colorize, argument can be auto, never or always
which defaults to auto.

For colored output in pipes use 'always', e.g.
  fdtdump -c always file.dtb | less -R

If you don't like colors use
alias fdtdump="fdtdump -c none"

Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
---
 Makefile  |  2 +-
 fdtdump.c | 37 +++++++++++++++++++++++++++++++++----
 util.c    | 12 +++++++-----
 util.h    |  8 ++++++++
 4 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index 32dcfcf..ee543b9 100644
--- a/Makefile
+++ b/Makefile
@@ -18,7 +18,7 @@ CONFIG_LOCALVERSION =
 CPPFLAGS = -I libfdt -I .
 WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
 	-Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
-CFLAGS = -g -Os -fPIC -Werror $(WARNINGS)
+CFLAGS = -g -Os -fPIC -Werror $(WARNINGS) -lncurses
 
 BISON = bison
 LEX = flex
diff --git a/fdtdump.c b/fdtdump.c
index 717fef5..59194cd 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -2,6 +2,9 @@
  * fdtdump.c - Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
  */
 
+#include <unistd.h>
+#include <curses.h>
+#include <term.h>
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
@@ -63,7 +66,7 @@ static void dump_blob(void *blob, bool debug)
 	depth = 0;
 	shift = 4;
 
-	printf("/dts-v1/;\n");
+	printf("%s/dts-v1/;\n", COLNONE);
 	printf("// magic:\t\t0x%x\n", fdt32_to_cpu(bph->magic));
 	printf("// totalsize:\t\t0x%x (%d)\n", totalsize, totalsize);
 	printf("// off_dt_struct:\t0x%x\n", off_dt);
@@ -104,10 +107,11 @@ static void dump_blob(void *blob, bool debug)
 			s = p;
 			p = PALIGN(p + strlen(s) + 1, 4);
 
+			printf("%s", COLNODE);
 			if (*s == '\0')
 				s = "/";
 
-			printf("%*s%s {\n", depth * shift, "", s);
+			printf("%*s%s%s {\n", depth * shift, "", s, COLNONE);
 
 			depth++;
 			continue;
@@ -139,7 +143,7 @@ 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);
+		printf("%*s%s%s%s", depth * shift, "", COLPROP, s, COLNONE);
 		utilfdt_print_data(t, sz);
 		printf(";\n");
 	}
@@ -147,18 +151,33 @@ static void dump_blob(void *blob, bool debug)
 
 /* 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[] = "c:ds" USAGE_COMMON_SHORT_OPTS;
 static struct option const usage_long_opts[] = {
+	{"color",            a_argument, NULL, 'c'},
 	{"debug",            no_argument, NULL, 'd'},
 	{"scan",             no_argument, NULL, 's'},
 	USAGE_COMMON_LONG_OPTS
 };
 static const char * const usage_opts_help[] = {
+	"Colorize, argument can be auto, never or always",
 	"Dump debug information while decoding the file",
 	"Scan for an embedded fdt in file",
 	USAGE_COMMON_OPTS_HELP
 };
 
+static void check_colored(void) {
+	int ret;
+
+	colored = 0;
+	if (isatty(STDOUT_FILENO) != 1)
+		return;
+	if (setupterm(NULL, STDOUT_FILENO, &ret) != OK || ret != 1)
+		return;
+	if (2 > tigetnum("colors"))
+		return;
+	colored = 1;
+}
+
 int main(int argc, char *argv[])
 {
 	int opt;
@@ -168,10 +187,20 @@ int main(int argc, char *argv[])
 	bool scan = false;
 	off_t len;
 
+	check_colored();
+
 	while ((opt = util_getopt_long()) != EOF) {
 		switch (opt) {
 		case_USAGE_COMMON_FLAGS
 
+		case 'c':
+			if (!strcmp("none", optarg))
+				colored = 0;
+			else if (!strcmp("always", optarg))
+				colored = 1;
+			else if (strcmp("auto", optarg))
+				usage("invalid argument for colored");
+			break;
 		case 'd':
 			debug = true;
 			break;
diff --git a/util.c b/util.c
index 3550f86..29fd6ad 100644
--- a/util.c
+++ b/util.c
@@ -36,6 +36,8 @@
 #include "util.h"
 #include "version_gen.h"
 
+int colored = 0;
+
 char *xstrdup(const char *s)
 {
 	int len = strlen(s) + 1;
@@ -389,7 +391,7 @@ void utilfdt_print_data(const char *data, int len)
 
 		s = data;
 		do {
-			printf("\"%s\"", s);
+			printf("\"%s%s%s\"", COLSTRG, s, COLNONE);
 			s += strlen(s) + 1;
 			if (s < data + len)
 				printf(", ");
@@ -398,17 +400,17 @@ void utilfdt_print_data(const char *data, int len)
 	} else if ((len % 4) == 0) {
 		const uint32_t *cell = (const uint32_t *)data;
 
-		printf(" = <");
+		printf(" = <%s", COLNUMB);
 		for (i = 0, len /= 4; i < len; i++)
 			printf("0x%08x%s", fdt32_to_cpu(cell[i]),
 			       i < (len - 1) ? " " : "");
-		printf(">");
+		printf("%s>", COLNONE);
 	} else {
 		const unsigned char *p = (const unsigned char *)data;
-		printf(" = [");
+		printf(" = [%s", COLBYTE);
 		for (i = 0; i < len; i++)
 			printf("%02x%s", *p++, i < len - 1 ? " " : "");
-		printf("]");
+		printf("%s]", COLNONE);
 	}
 }
 
diff --git a/util.h b/util.h
index f5c4f1b..b814872 100644
--- a/util.h
+++ b/util.h
@@ -25,6 +25,14 @@
  *                                                                   USA
  */
 
+extern int colored;
+#define COLNODE (colored ? "\x1b[35m" : "") /* magenta */
+#define COLPROP (colored ? "\x1b[32m" : "") /* green */
+#define COLSTRG (colored ? "\x1b[33m" : "") /* yellow */
+#define COLNUMB (colored ? "\x1b[34m" : "") /* blue */
+#define COLBYTE (colored ? "\x1b[36m" : "") /* cyan */
+#define COLNONE (colored ? "\x1b[0m" : "")  /* default */
+
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 
 static inline void __attribute__((noreturn)) die(const char *str, ...)
-- 
2.11.0

Re: [PATCH 1/1] fdtdump.c: provide colored output

[Simon Glass]

Hi Heinrich,

On 22 December 2016 at 10:54, Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org> wrote:
> A new command line option is added
>   -c, --color <arg> Colorize, argument can be auto, never or always
> which defaults to auto.
>
> For colored output in pipes use 'always', e.g.
>   fdtdump -c always file.dtb | less -R
>
> If you don't like colors use
> alias fdtdump="fdtdump -c none"
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
> ---
>  Makefile  |  2 +-
>  fdtdump.c | 37 +++++++++++++++++++++++++++++++++----
>  util.c    | 12 +++++++-----
>  util.h    |  8 ++++++++
>  4 files changed, 49 insertions(+), 10 deletions(-)
>

Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

But please see below.

> diff --git a/Makefile b/Makefile
> index 32dcfcf..ee543b9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -18,7 +18,7 @@ CONFIG_LOCALVERSION =
>  CPPFLAGS = -I libfdt -I .
>  WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
>         -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
> -CFLAGS = -g -Os -fPIC -Werror $(WARNINGS)
> +CFLAGS = -g -Os -fPIC -Werror $(WARNINGS) -lncurses
>
>  BISON = bison
>  LEX = flex
> diff --git a/fdtdump.c b/fdtdump.c
> index 717fef5..59194cd 100644
> --- a/fdtdump.c
> +++ b/fdtdump.c
> @@ -2,6 +2,9 @@
>   * fdtdump.c - Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
>   */
>
> +#include <unistd.h>
> +#include <curses.h>
> +#include <term.h>
>  #include <stdbool.h>
>  #include <stdint.h>
>  #include <stdio.h>
> @@ -63,7 +66,7 @@ static void dump_blob(void *blob, bool debug)
>         depth = 0;
>         shift = 4;
>
> -       printf("/dts-v1/;\n");
> +       printf("%s/dts-v1/;\n", COLNONE);
>         printf("// magic:\t\t0x%x\n", fdt32_to_cpu(bph->magic));
>         printf("// totalsize:\t\t0x%x (%d)\n", totalsize, totalsize);
>         printf("// off_dt_struct:\t0x%x\n", off_dt);
> @@ -104,10 +107,11 @@ static void dump_blob(void *blob, bool debug)
>                         s = p;
>                         p = PALIGN(p + strlen(s) + 1, 4);
>
> +                       printf("%s", COLNODE);

Another option would be to make COLNODE a function that returns either
"" or an ANSI string, depending on the setting of 'colored'. Then you
can avoid the global variable.

E.g.:

char str[ANSI_LEN];

                      printf("%s", get_ansi(str, COL_NODE));

>                         if (*s == '\0')
>                                 s = "/";
>

Regards,
Simon

Re: [PATCH 1/1] fdtdump.c: provide colored output

[David Gibson]

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

On Wed, Dec 21, 2016 at 10:54:31PM +0100, Heinrich Schuchardt wrote:
> A new command line option is added
>   -c, --color <arg> Colorize, argument can be auto, never or always
> which defaults to auto.
> 
> For colored output in pipes use 'always', e.g.
>   fdtdump -c always file.dtb | less -R
> 
> If you don't like colors use
> alias fdtdump="fdtdump -c none"
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>

Hmm.  So, as I said, I really see fdtdump as a minimal hack.  For real
life use I recommend dtc -I dtb -O dts instead.  So, I'm not thrilled
about adding a feature like this to fdtdump but not to dtc's dts
output.

> ---
>  Makefile  |  2 +-
>  fdtdump.c | 37 +++++++++++++++++++++++++++++++++----
>  util.c    | 12 +++++++-----
>  util.h    |  8 ++++++++
>  4 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 32dcfcf..ee543b9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -18,7 +18,7 @@ CONFIG_LOCALVERSION =
>  CPPFLAGS = -I libfdt -I .
>  WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
>  	-Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
> -CFLAGS = -g -Os -fPIC -Werror $(WARNINGS)
> +CFLAGS = -g -Os -fPIC -Werror $(WARNINGS) -lncurses
>  
>  BISON = bison
>  LEX = flex
> diff --git a/fdtdump.c b/fdtdump.c
> index 717fef5..59194cd 100644
> --- a/fdtdump.c
> +++ b/fdtdump.c
> @@ -2,6 +2,9 @@
>   * fdtdump.c - Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
>   */
>  
> +#include <unistd.h>
> +#include <curses.h>
> +#include <term.h>
>  #include <stdbool.h>
>  #include <stdint.h>
>  #include <stdio.h>
> @@ -63,7 +66,7 @@ static void dump_blob(void *blob, bool debug)
>  	depth = 0;
>  	shift = 4;
>  
> -	printf("/dts-v1/;\n");
> +	printf("%s/dts-v1/;\n", COLNONE);
>  	printf("// magic:\t\t0x%x\n", fdt32_to_cpu(bph->magic));
>  	printf("// totalsize:\t\t0x%x (%d)\n", totalsize, totalsize);
>  	printf("// off_dt_struct:\t0x%x\n", off_dt);
> @@ -104,10 +107,11 @@ static void dump_blob(void *blob, bool debug)
>  			s = p;
>  			p = PALIGN(p + strlen(s) + 1, 4);
>  
> +			printf("%s", COLNODE);
>  			if (*s == '\0')
>  				s = "/";
>  
> -			printf("%*s%s {\n", depth * shift, "", s);
> +			printf("%*s%s%s {\n", depth * shift, "", s, COLNONE);
>  
>  			depth++;
>  			continue;
> @@ -139,7 +143,7 @@ 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);
> +		printf("%*s%s%s%s", depth * shift, "", COLPROP, s, COLNONE);
>  		utilfdt_print_data(t, sz);
>  		printf(";\n");
>  	}
> @@ -147,18 +151,33 @@ static void dump_blob(void *blob, bool debug)
>  
>  /* 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[] = "c:ds" USAGE_COMMON_SHORT_OPTS;
>  static struct option const usage_long_opts[] = {
> +	{"color",            a_argument, NULL, 'c'},
>  	{"debug",            no_argument, NULL, 'd'},
>  	{"scan",             no_argument, NULL, 's'},
>  	USAGE_COMMON_LONG_OPTS
>  };
>  static const char * const usage_opts_help[] = {
> +	"Colorize, argument can be auto, never or always",
>  	"Dump debug information while decoding the file",
>  	"Scan for an embedded fdt in file",
>  	USAGE_COMMON_OPTS_HELP
>  };
>  
> +static void check_colored(void) {
> +	int ret;
> +
> +	colored = 0;
> +	if (isatty(STDOUT_FILENO) != 1)
> +		return;
> +	if (setupterm(NULL, STDOUT_FILENO, &ret) != OK || ret != 1)
> +		return;
> +	if (2 > tigetnum("colors"))
> +		return;
> +	colored = 1;
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	int opt;
> @@ -168,10 +187,20 @@ int main(int argc, char *argv[])
>  	bool scan = false;
>  	off_t len;
>  
> +	check_colored();
> +
>  	while ((opt = util_getopt_long()) != EOF) {
>  		switch (opt) {
>  		case_USAGE_COMMON_FLAGS
>  
> +		case 'c':
> +			if (!strcmp("none", optarg))
> +				colored = 0;
> +			else if (!strcmp("always", optarg))
> +				colored = 1;
> +			else if (strcmp("auto", optarg))
> +				usage("invalid argument for colored");
> +			break;
>  		case 'd':
>  			debug = true;
>  			break;
> diff --git a/util.c b/util.c
> index 3550f86..29fd6ad 100644
> --- a/util.c
> +++ b/util.c
> @@ -36,6 +36,8 @@
>  #include "util.h"
>  #include "version_gen.h"
>  
> +int colored = 0;
> +
>  char *xstrdup(const char *s)
>  {
>  	int len = strlen(s) + 1;
> @@ -389,7 +391,7 @@ void utilfdt_print_data(const char *data, int len)
>  
>  		s = data;
>  		do {
> -			printf("\"%s\"", s);
> +			printf("\"%s%s%s\"", COLSTRG, s, COLNONE);
>  			s += strlen(s) + 1;
>  			if (s < data + len)
>  				printf(", ");
> @@ -398,17 +400,17 @@ void utilfdt_print_data(const char *data, int len)
>  	} else if ((len % 4) == 0) {
>  		const uint32_t *cell = (const uint32_t *)data;
>  
> -		printf(" = <");
> +		printf(" = <%s", COLNUMB);
>  		for (i = 0, len /= 4; i < len; i++)
>  			printf("0x%08x%s", fdt32_to_cpu(cell[i]),
>  			       i < (len - 1) ? " " : "");
> -		printf(">");
> +		printf("%s>", COLNONE);
>  	} else {
>  		const unsigned char *p = (const unsigned char *)data;
> -		printf(" = [");
> +		printf(" = [%s", COLBYTE);
>  		for (i = 0; i < len; i++)
>  			printf("%02x%s", *p++, i < len - 1 ? " " : "");
> -		printf("]");
> +		printf("%s]", COLNONE);
>  	}
>  }
>  
> diff --git a/util.h b/util.h
> index f5c4f1b..b814872 100644
> --- a/util.h
> +++ b/util.h
> @@ -25,6 +25,14 @@
>   *                                                                   USA
>   */
>  
> +extern int colored;
> +#define COLNODE (colored ? "\x1b[35m" : "") /* magenta */
> +#define COLPROP (colored ? "\x1b[32m" : "") /* green */
> +#define COLSTRG (colored ? "\x1b[33m" : "") /* yellow */
> +#define COLNUMB (colored ? "\x1b[34m" : "") /* blue */
> +#define COLBYTE (colored ? "\x1b[36m" : "") /* cyan */
> +#define COLNONE (colored ? "\x1b[0m" : "")  /* default */

Um.. this looks absolutely wrong.  You've gone to the trouble of
including the ncurses library, but you're basically not using it
except for the inital on/off check.  Here you've used fixed explicit
escape codes, which will only be correct for one terminal type -
terminfo and ncurses basically exist for abstracting this sort of thing.

>  #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>  
>  static inline void __attribute__((noreturn)) die(const char *str, ...)

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