[PATCH 00/10 v2] usage()/--help clean up & unification

[PATCH 00/10 v2] usage()/--help clean up & unification

[Mike Frysinger]

Each utility currently open codes how they process options and
implement usage().  This leads to inconsistent behavior and output
which sucks.  A bunch also get common things wrong (like sending
--help to stderr).

Rather than go through them one by one and fix their bugs, start
a new mini framework in the util.[ch] code base.  This allows the
utils to be much more data driven in their approach to options --
they declare certain structures with the right format, and then
call usage() as needed.  The new util code takes care of the rest.

I also snuck in a few minor fixes in this patch series that I
noticed as I was hacking on things.  Really I just wanted a --scan
option in the fdtdump tool :).

For the v2 series, I fixed a compile/output bug in one of the
patches and this now passes `make check`.  I also added a --debug
option to fdtdump.

Mike Frysinger (10):
  utilfdt_read_err: use xmalloc funcs
  utilfdt_read: pass back up the length of data read
  die: constify format string arg
  util_version: new helper for displaying version info
  fdtdump: make usage a bit more friendly
  fdtdump: add a --scan option
  dtc/fdt{get,put}/convert-dtsv0-lexer: convert to new usage helpers
  util: drop "long" from usage helpers
  util: add common ARRAY_SIZE define
  fdtdump: add a debug mode

 convert-dtsv0-lexer.l |  24 +++++++----
 dtc.c                 | 116 +++++++++++++++++++++++++-------------------------
 dtc.h                 |   1 -
 fdtdump.c             | 109 ++++++++++++++++++++++++++++++++++++++++++-----
 fdtget.c              |  62 ++++++++++++---------------
 fdtput.c              |  65 +++++++++++++---------------
 tests/testutils.c     |   2 +-
 util.c                |  73 ++++++++++++++++++++++++++++---
 util.h                |  78 +++++++++++++++++++++++++++++++--
 9 files changed, 375 insertions(+), 155 deletions(-)

-- 
1.8.1.2

[PATCH 01/10] utilfdt_read_err: use xmalloc funcs

[Mike Frysinger]

We've got these handy helpers, so let's use them.

Acked-by: David Gibson <David-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util.c b/util.c
index b081fa8..12f0614 100644
--- a/util.c
+++ b/util.c
@@ -212,12 +212,12 @@ int utilfdt_read_err(const char *filename, char **buffp)
 	}
 
 	/* Loop until we have read everything */
-	buf = malloc(bufsize);
+	buf = xmalloc(bufsize);
 	do {
 		/* Expand the buffer to hold the next chunk */
 		if (offset == bufsize) {
 			bufsize *= 2;
-			buf = realloc(buf, bufsize);
+			buf = xrealloc(buf, bufsize);
 			if (!buf) {
 				ret = ENOMEM;
 				break;
-- 
1.8.1.2

[PATCH 02/10] utilfdt_read: pass back up the length of data read

[Mike Frysinger]

For a follow up commit, we want to be able to scan the buffer that was
returned to us.  In order to do that safely, we need to know how big
the buffer actually is, so pass that back if requested.

Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 fdtget.c          | 2 +-
 fdtput.c          | 2 +-
 tests/testutils.c | 2 +-
 util.c            | 8 +++++---
 util.h            | 6 ++++--
 5 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/fdtget.c b/fdtget.c
index c2fbab2..374bee2 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -254,7 +254,7 @@ static int do_fdtget(struct display_info *disp, const char *filename,
 	const char *prop;
 	int i, node;
 
-	blob = utilfdt_read(filename);
+	blob = utilfdt_read(filename, NULL);
 	if (!blob)
 		return -1;
 
diff --git a/fdtput.c b/fdtput.c
index f2197f5..572d6da 100644
--- a/fdtput.c
+++ b/fdtput.c
@@ -239,7 +239,7 @@ static int do_fdtput(struct display_info *disp, const char *filename,
 	char *blob;
 	int len, ret = 0;
 
-	blob = utilfdt_read(filename);
+	blob = utilfdt_read(filename, NULL);
 	if (!blob)
 		return -1;
 
diff --git a/tests/testutils.c b/tests/testutils.c
index f185133..67b39b0 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -160,7 +160,7 @@ int nodename_eq(const char *s1, const char *s2)
 void *load_blob(const char *filename)
 {
 	char *blob;
-	int ret = utilfdt_read_err(filename, &blob);
+	int ret = utilfdt_read_err(filename, &blob, NULL);
 
 	if (ret)
 		CONFIG("Couldn't open blob from \"%s\": %s", filename,
diff --git a/util.c b/util.c
index 12f0614..c689ea7 100644
--- a/util.c
+++ b/util.c
@@ -197,7 +197,7 @@ char get_escape_char(const char *s, int *i)
 	return val;
 }
 
-int utilfdt_read_err(const char *filename, char **buffp)
+int utilfdt_read_err(const char *filename, char **buffp, off_t *len)
 {
 	int fd = 0;	/* assume stdin */
 	char *buf = NULL;
@@ -238,13 +238,15 @@ int utilfdt_read_err(const char *filename, char **buffp)
 		free(buf);
 	else
 		*buffp = buf;
+	if (len)
+		*len = bufsize;
 	return ret;
 }
 
-char *utilfdt_read(const char *filename)
+char *utilfdt_read(const char *filename, off_t *len)
 {
 	char *buff;
-	int ret = utilfdt_read_err(filename, &buff);
+	int ret = utilfdt_read_err(filename, &buff, len);
 
 	if (ret) {
 		fprintf(stderr, "Couldn't open blob from '%s': %s\n", filename,
diff --git a/util.h b/util.h
index 543a173..3c20b15 100644
--- a/util.h
+++ b/util.h
@@ -80,9 +80,10 @@ char get_escape_char(const char *s, int *i);
  * stderr.
  *
  * @param filename	The filename to read, or - for stdin
+ * @param len		If non-NULL, the amount of data we managed to read
  * @return Pointer to allocated buffer containing fdt, or NULL on error
  */
-char *utilfdt_read(const char *filename);
+char *utilfdt_read(const char *filename, off_t *len);
 
 /**
  * Read a device tree file into a buffer. Does not report errors, but only
@@ -91,9 +92,10 @@ char *utilfdt_read(const char *filename);
  *
  * @param filename	The filename to read, or - for stdin
  * @param buffp		Returns pointer to buffer containing fdt
+ * @param len		If non-NULL, the amount of data we managed to read
  * @return 0 if ok, else an errno value representing the error
  */
-int utilfdt_read_err(const char *filename, char **buffp);
+int utilfdt_read_err(const char *filename, char **buffp, off_t *len);
 
 
 /**
-- 
1.8.1.2

Re: [PATCH 02/10] utilfdt_read: pass back up the length of data read

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 716 bytes --]

On Wed, Apr 10, 2013 at 02:29:07PM -0400, Mike Frysinger wrote:
> For a follow up commit, we want to be able to scan the buffer that was
> returned to us.  In order to do that safely, we need to know how big
> the buffer actually is, so pass that back if requested.

Rather than changing all the callers, I'd prefer to see a new function
implemented.  The existing utilfdt_read(), which assumes the caller
can get the size from the fdt header, can be reimplemented in terms of
the version returning the length.

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 02/10] utilfdt_read: pass back up the length of data read

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 697 bytes --]

On Monday 15 April 2013 00:58:38 David Gibson wrote:
> On Wed, Apr 10, 2013 at 02:29:07PM -0400, Mike Frysinger wrote:
> > For a follow up commit, we want to be able to scan the buffer that was
> > returned to us.  In order to do that safely, we need to know how big
> > the buffer actually is, so pass that back if requested.
> 
> Rather than changing all the callers, I'd prefer to see a new function
> implemented.  The existing utilfdt_read(), which assumes the caller
> can get the size from the fdt header, can be reimplemented in terms of
> the version returning the length.

ok, i've added stub funcs that take a _len param and rebase the existing ones 
on top of that
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 03/10] die: constify format string arg

[Mike Frysinger]

We only display this string, so there's no need for it to be writable.
Constify away!

Acked-by: David Gibson <David-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 util.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util.h b/util.h
index 3c20b15..d856eb9 100644
--- a/util.h
+++ b/util.h
@@ -23,7 +23,7 @@
  *                                                                   USA
  */
 
-static inline void __attribute__((noreturn)) die(char * str, ...)
+static inline void __attribute__((noreturn)) die(const char *str, ...)
 {
 	va_list ap;
 
-- 
1.8.1.2

[PATCH 04/10] util_version: new helper for displaying version info

[Mike Frysinger]

This is so all utilities can have this flag and not just dtc.

Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 dtc.c  | 5 +----
 util.c | 7 +++++++
 util.h | 5 +++++
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/dtc.c b/dtc.c
index d40e220..e4e1b84 100644
--- a/dtc.c
+++ b/dtc.c
@@ -21,8 +21,6 @@
 #include "dtc.h"
 #include "srcpos.h"
 
-#include "version_gen.h"
-
 /*
  * Command line options
  */
@@ -158,8 +156,7 @@ int main(int argc, char *argv[])
 			srcfile_add_search_path(optarg);
 			break;
 		case 'v':
-			printf("Version: %s\n", DTC_VERSION);
-			exit(0);
+			util_version();
 		case 'H':
 			if (streq(optarg, "legacy"))
 				phandle_format = PHANDLE_LEGACY;
diff --git a/util.c b/util.c
index c689ea7..a9b118c 100644
--- a/util.c
+++ b/util.c
@@ -34,6 +34,7 @@
 
 #include "libfdt.h"
 #include "util.h"
+#include "version_gen.h"
 
 char *xstrdup(const char *s)
 {
@@ -374,3 +375,9 @@ void utilfdt_print_data(const char *data, int len)
 		printf("]");
 	}
 }
+
+void util_version(void)
+{
+	printf("Version: %s\n", DTC_VERSION);
+	exit(0);
+}
diff --git a/util.h b/util.h
index d856eb9..e09cc25 100644
--- a/util.h
+++ b/util.h
@@ -168,4 +168,9 @@ int utilfdt_decode_type(const char *fmt, int *type, int *size);
  */
 void utilfdt_print_data(const char *data, int len);
 
+/**
+ * Show source version and exit
+ */
+void util_version(void) __attribute__((noreturn));
+
 #endif /* _UTIL_H */
-- 
1.8.1.2

[PATCH 05/10] fdtdump: make usage a bit more friendly

[Mike Frysinger]

This starts a new usage framework and then cuts fdtdump over to it.
Now we can do `fdtdump -h` and get something useful back.

Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 fdtdump.c | 31 +++++++++++++++++++++++--------
 util.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 util.h    | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+), 8 deletions(-)

diff --git a/fdtdump.c b/fdtdump.c
index 03ea429..b685ed1 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -117,21 +117,36 @@ static void dump_blob(void *blob)
 	}
 }
 
+/* Usage related data. */
+static const char usage_synopsis[] = "fdtdump [options] <file>";
+static const char usage_short_opts[] = USAGE_COMMON_SHORT_OPTS;
+static struct option const usage_long_opts[] = {
+	USAGE_COMMON_LONG_OPTS
+};
+static const char * const usage_opts_help[] = {
+	USAGE_COMMON_OPTS_HELP
+};
 
 int main(int argc, char *argv[])
 {
+	int opt;
+	const char *file;
 	char *buf;
 
-	if (argc < 2) {
-		fprintf(stderr, "supply input filename\n");
-		return 5;
+	while ((opt = util_getopt_long()) != EOF) {
+		switch (opt) {
+		case_USAGE_COMMON_FLAGS
+		}
 	}
+	if (optind != argc - 1)
+		long_usage("missing input filename");
+	file = argv[optind];
+
+	buf = utilfdt_read(file, NULL);
+	if (!buf)
+		die("could not read: %s\n", file);
 
-	buf = utilfdt_read(argv[1]);
-	if (buf)
-		dump_blob(buf);
-	else
-		return 10;
+	dump_blob(buf);
 
 	return 0;
 }
diff --git a/util.c b/util.c
index a9b118c..8d427e4 100644
--- a/util.c
+++ b/util.c
@@ -381,3 +381,57 @@ void util_version(void)
 	printf("Version: %s\n", DTC_VERSION);
 	exit(0);
 }
+
+void util_long_usage(const char *errmsg, const char *synopsis,
+		     const char *short_opts, struct option const long_opts[],
+		     const char * const opts_help[])
+{
+	FILE *fp = errmsg ? stderr : stdout;
+	const char a_arg[] = "<arg>";
+	size_t a_arg_len = strlen(a_arg) + 1;
+	size_t i;
+	int optlen;
+
+	fprintf(fp,
+		"Usage: %s\n"
+		"\n"
+		"Options: -[%s]\n", synopsis, short_opts);
+
+	/* prescan the --long opt length to auto-align */
+	optlen = 0;
+	for (i = 0; long_opts[i].name; ++i) {
+		/* +1 is for space between --opt and help text */
+		int l = strlen(long_opts[i].name) + 1;
+		if (long_opts[i].has_arg == a_argument)
+			l += a_arg_len;
+		if (optlen < l)
+			optlen = l;
+	}
+
+	for (i = 0; long_opts[i].name; ++i) {
+		/* helps when adding new applets or options */
+		assert(opts_help[i] != NULL);
+
+		/* first output the short flag if it has one */
+		if (long_opts[i].val > '~')
+			fprintf(fp, "      ");
+		else
+			fprintf(fp, "  -%c, ", long_opts[i].val);
+
+		/* then the long flag */
+		if (long_opts[i].has_arg == no_argument)
+			fprintf(fp, "--%-*s", optlen, long_opts[i].name);
+		else
+			fprintf(fp, "--%s %s%*s", long_opts[i].name, a_arg,
+				(int)(optlen - strlen(long_opts[i].name) - a_arg_len), "");
+
+		/* finally the help text */
+		fprintf(fp, "%s\n", opts_help[i]);
+	}
+
+	if (errmsg) {
+		fprintf(fp, "\nError: %s\n", errmsg);
+		exit(EXIT_FAILURE);
+	} else
+		exit(EXIT_SUCCESS);
+}
diff --git a/util.h b/util.h
index e09cc25..f9bc777 100644
--- a/util.h
+++ b/util.h
@@ -2,6 +2,7 @@
 #define _UTIL_H
 
 #include <stdarg.h>
+#include <getopt.h>
 
 /*
  * Copyright 2011 The Chromium Authors, All Rights Reserved.
@@ -173,4 +174,64 @@ void utilfdt_print_data(const char *data, int len);
  */
 void util_version(void) __attribute__((noreturn));
 
+/**
+ * Show usage and exit
+ *
+ * This helps standardize the output of various utils.  You most likely want
+ * to use the long_usage() helper below rather than call this.
+ *
+ * @param errmsg	If non-NULL, an error message to display
+ * @param synopsis	The initial example usage text (and possible examples)
+ * @param short_opts	The string of short options
+ * @param long_opts	The structure of long options
+ * @param opts_help	An array of help strings (should align with long_opts)
+ */
+void util_long_usage(const char *errmsg, const char *synopsis,
+		     const char *short_opts, struct option const long_opts[],
+		     const char * const opts_help[]) __attribute__((noreturn));
+
+/**
+ * Show usage and exit
+ *
+ * If you name all your usage variables with usage_xxx, then you can call this
+ * help macro rather than expanding all arguments yourself.
+ *
+ * @param errmsg	If non-NULL, an error message to display
+ */
+#define long_usage(errmsg) \
+	util_long_usage(errmsg, usage_synopsis, usage_short_opts, \
+			usage_long_opts, usage_opts_help)
+
+/**
+ * Call getopt_long() with standard options
+ *
+ * Since all util code runs getopt in the same way, provide a helper.
+ */
+#define util_getopt_long() getopt_long(argc, argv, usage_short_opts, \
+				       usage_long_opts, NULL)
+
+/* Helper for aligning long_opts array */
+#define a_argument required_argument
+
+/* Helper for usage_short_opts string constant */
+#define USAGE_COMMON_SHORT_OPTS "hV"
+
+/* Helper for usage_long_opts option array */
+#define USAGE_COMMON_LONG_OPTS \
+	{"help",      no_argument, NULL, 'h'}, \
+	{"version",   no_argument, NULL, 'V'}, \
+	{NULL,        no_argument, NULL, 0x0}
+
+/* Helper for usage_opts_help array */
+#define USAGE_COMMON_OPTS_HELP \
+	"Print this help and exit", \
+	"Print version and exit", \
+	NULL
+
+/* Helper for getopt case statements */
+#define case_USAGE_COMMON_FLAGS \
+	case 'h': long_usage(NULL); \
+	case 'V': util_version(); \
+	case '?': long_usage("unknown option");
+
 #endif /* _UTIL_H */
-- 
1.8.1.2

Re: [PATCH 05/10] fdtdump: make usage a bit more friendly

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 573 bytes --]

On Wed, Apr 10, 2013 at 02:29:10PM -0400, Mike Frysinger wrote:
> This starts a new usage framework and then cuts fdtdump over to it.
> Now we can do `fdtdump -h` and get something useful back.

Hrm.  The problem is that the new scheme relies on getopt_long(),
which is a GNU extension.  I'd like to have dtc and associated tools
compilable on FreeBSD as well as Linux.

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 05/10] fdtdump: make usage a bit more friendly

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 1116 bytes --]

On Thursday 11 April 2013 00:00:14 David Gibson wrote:
> On Wed, Apr 10, 2013 at 02:29:10PM -0400, Mike Frysinger wrote:
> > This starts a new usage framework and then cuts fdtdump over to it.
> > Now we can do `fdtdump -h` and get something useful back.
> 
> Hrm.  The problem is that the new scheme relies on getopt_long(),
> which is a GNU extension.  I'd like to have dtc and associated tools
> compilable on FreeBSD as well as Linux.

i don't run FreeBSD myself, but according to their own website, it looks like
getopt_long() is available ?
http://www.freebsd.org/cgi/man.cgi?query=getopt_long&apropos=0&sektion=3&manpath=FreeBSD+9.1-
RELEASE&arch=default&format=html

HISTORY
     The getopt_long() and getopt_long_only() functions first appeared in the
     GNU libiberty library.  The first BSD implementation of getopt_long()
     appeared in NetBSD 1.5, the first BSD implementation of
     getopt_long_only() in OpenBSD 3.3.  FreeBSD first included getopt_long()
     in FreeBSD 5.0, getopt_long_only() in FreeBSD 5.2.

i'm assuming you don't need to support <FreeBSD 5.0 :)
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 06/10] fdtdump: add a --scan option

[Mike Frysinger]

Often times, fdts get embedded in other larger files.  Rather than force
people to `dd` the blob out themselves, make the fdtdump file smarter.

It can now scan the blob looking for the fdt magic.  Once locate, it does
a little validation on the main struct to make sure we didn't hit random
binary data.

Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 fdtdump.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/fdtdump.c b/fdtdump.c
index b685ed1..7b967aa 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -2,12 +2,14 @@
  * fdtdump.c - Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
  */
 
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
 
+#include <libfdt.h>
 #include <libfdt_env.h>
 #include <fdt.h>
 
@@ -119,11 +121,13 @@ static void dump_blob(void *blob)
 
 /* Usage related data. */
 static const char usage_synopsis[] = "fdtdump [options] <file>";
-static const char usage_short_opts[] = USAGE_COMMON_SHORT_OPTS;
+static const char usage_short_opts[] = "s" USAGE_COMMON_SHORT_OPTS;
 static struct option const usage_long_opts[] = {
+	{"scan",             no_argument, NULL, 's'},
 	USAGE_COMMON_LONG_OPTS
 };
 static const char * const usage_opts_help[] = {
+	"Scan for an embedded fdt in file",
 	USAGE_COMMON_OPTS_HELP
 };
 
@@ -132,20 +136,58 @@ int main(int argc, char *argv[])
 	int opt;
 	const char *file;
 	char *buf;
+	bool scan = false;
+	off_t len;
 
 	while ((opt = util_getopt_long()) != EOF) {
 		switch (opt) {
 		case_USAGE_COMMON_FLAGS
+
+		case 's':
+			scan = true;
+			break;
 		}
 	}
 	if (optind != argc - 1)
 		long_usage("missing input filename");
 	file = argv[optind];
 
-	buf = utilfdt_read(file, NULL);
+	buf = utilfdt_read(file, &len);
 	if (!buf)
 		die("could not read: %s\n", file);
 
+	/* try and locate an embedded fdt in a bigger blob */
+	if (scan) {
+		unsigned char smagic[4];
+		char *p = buf;
+		char *endp = buf + len;
+
+		fdt_set_magic(smagic, FDT_MAGIC);
+
+		/* poor man's memmem */
+		while (true) {
+			p = memchr(p, smagic[0], endp - p - 4);
+			if (!p)
+				break;
+			if (fdt_magic(p) == FDT_MAGIC) {
+				/* try and validate the main struct */
+				off_t this_len = endp - p;
+				fdt32_t max_version = 17;
+				if (fdt_version(p) <= max_version &&
+				    fdt_last_comp_version(p) < max_version &&
+				    fdt_totalsize(p) < this_len &&
+				    fdt_off_dt_struct(p) < this_len &&
+					fdt_off_dt_strings(p) < this_len)
+					break;
+			}
+			++p;
+		}
+		if (!p)
+			die("%s: could not locate fdt magic\n", file);
+		printf("%s: found fdt at offset %#zx\n", file, p - buf);
+		buf = p;
+	}
+
 	dump_blob(buf);
 
 	return 0;
-- 
1.8.1.2

Re: [PATCH 06/10] fdtdump: add a --scan option

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 1228 bytes --]

On Wed, Apr 10, 2013 at 02:29:11PM -0400, Mike Frysinger wrote:
> Often times, fdts get embedded in other larger files.  Rather than force
> people to `dd` the blob out themselves, make the fdtdump file smarter.
> 
> It can now scan the blob looking for the fdt magic.  Once locate, it does
> a little validation on the main struct to make sure we didn't hit random
> binary data.
> 
> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>

Hrm.  I have mixed feelings about this.  The scanning functionality is
certainly useful.  But on the other hand, fdtdump is not supposed to
be a general use tool.  It's basically a debugging aid: a quick and
dirty independent implementation of a dtb dump, for checking that dtc
is producing sane output.  The preferred way to dump dtbs "for real"
is to use dtc -I dtb -O dts

I think the way I'd prefer to see this functionality would be to add a
new input format to dtc which is a little wrapper around the dtb input
format which does the scan.

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 06/10] fdtdump: add a --scan option

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 1972 bytes --]

On Thursday 11 April 2013 00:11:30 David Gibson wrote:
> On Wed, Apr 10, 2013 at 02:29:11PM -0400, Mike Frysinger wrote:
> > Often times, fdts get embedded in other larger files.  Rather than force
> > people to `dd` the blob out themselves, make the fdtdump file smarter.
> > 
> > It can now scan the blob looking for the fdt magic.  Once locate, it does
> > a little validation on the main struct to make sure we didn't hit random
> > binary data.
> 
> Hrm.  I have mixed feelings about this.  The scanning functionality is
> certainly useful.  But on the other hand, fdtdump is not supposed to
> be a general use tool.  It's basically a debugging aid: a quick and
> dirty independent implementation of a dtb dump, for checking that dtc
> is producing sane output.  The preferred way to dump dtbs "for real"
> is to use dtc -I dtb -O dts
> 
> I think the way I'd prefer to see this functionality would be to add a
> new input format to dtc which is a little wrapper around the dtb input
> format which does the scan.

that seems reasonable, but really my use case was: scan a big blob, find the 
dtb embedded in there, then both decode and dump the offsets.  this was so i 
could easily hack said big blob with a hexeditor and tweak a few keys.

adding a new input format would satisfy the first part (locating the dtb), and 
the fact that dtc already supports "-O dtb" is good as it'd allow me to run it 
through fdtdump after the fact.  i can probably even chain them in a pipeline.

i'd still want the --debug option in fdtdump, but it sounds like that wouldn't 
be against your desired goal for this utility ?

if that sounds good to you, i can rework things so that dtc gets a new "scan" 
input format (guessing i don't need to name it "scan-dtb").  then we both 
should be happy.

on a related note, is there a reason these tools malloc & read in the whole 
file into memory instead of just doing mmap() when possible ?
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 06/10] fdtdump: add a --scan option

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 2777 bytes --]

On Thu, Apr 11, 2013 at 06:44:20PM -0400, Mike Frysinger wrote:
> On Thursday 11 April 2013 00:11:30 David Gibson wrote:
> > On Wed, Apr 10, 2013 at 02:29:11PM -0400, Mike Frysinger wrote:
> > > Often times, fdts get embedded in other larger files.  Rather than force
> > > people to `dd` the blob out themselves, make the fdtdump file smarter.
> > > 
> > > It can now scan the blob looking for the fdt magic.  Once locate, it does
> > > a little validation on the main struct to make sure we didn't hit random
> > > binary data.
> > 
> > Hrm.  I have mixed feelings about this.  The scanning functionality is
> > certainly useful.  But on the other hand, fdtdump is not supposed to
> > be a general use tool.  It's basically a debugging aid: a quick and
> > dirty independent implementation of a dtb dump, for checking that dtc
> > is producing sane output.  The preferred way to dump dtbs "for real"
> > is to use dtc -I dtb -O dts
> > 
> > I think the way I'd prefer to see this functionality would be to add a
> > new input format to dtc which is a little wrapper around the dtb input
> > format which does the scan.
> 
> that seems reasonable, but really my use case was: scan a big blob, find the 
> dtb embedded in there, then both decode and dump the offsets.  this was so i 
> could easily hack said big blob with a hexeditor and tweak a few
> keys.

Yeah, so when I got to your debug patch, I already started changing my
mind here.

> adding a new input format would satisfy the first part (locating the dtb), and 
> the fact that dtc already supports "-O dtb" is good as it'd allow me to run it 
> through fdtdump after the fact.  i can probably even chain them in a pipeline.
> 
> i'd still want the --debug option in fdtdump, but it sounds like that wouldn't 
> be against your desired goal for this utility ?
> 
> if that sounds good to you, i can rework things so that dtc gets a new "scan" 
> input format (guessing i don't need to name it "scan-dtb").  then we both 
> should be happy.

Actually, you've convinced me.  So for now, just go back to your
original approach and address the other more specific comments and
that should be fine.  When I made that suggestion I hadn't realised
what you were using this for.  Now that I have, I think it fits into
fdtdump as intended.

> on a related note, is there a reason these tools malloc & read in the whole 
> file into memory instead of just doing mmap() when possible ?

Some combination of simplicity and portability.  I've never really
thought about it much..

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 07/10] dtc/fdt{get, put}/convert-dtsv0-lexer: convert to new usage helpers

[Mike Frysinger]

This helps standardize the flag processing and the usage screens.

Only lightly tested; would be great if someone who uses these utils
could double check.

Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 convert-dtsv0-lexer.l |  22 +++++++---
 dtc.c                 | 111 ++++++++++++++++++++++++++------------------------
 fdtget.c              |  60 ++++++++++++---------------
 fdtput.c              |  63 +++++++++++++---------------
 util.h                |   2 +-
 5 files changed, 131 insertions(+), 127 deletions(-)

diff --git a/convert-dtsv0-lexer.l b/convert-dtsv0-lexer.l
index 89d540a..e62d27a 100644
--- a/convert-dtsv0-lexer.l
+++ b/convert-dtsv0-lexer.l
@@ -194,11 +194,15 @@ const struct {
 		}
 
 %%
-static void usage(void)
-{
-	fprintf(stderr, "convert-dtsv0 <v0 dts file>...\n");
-	exit(3);
-}
+/* Usage related data. */
+static const char usage_synopsis[] = "convert-dtsv0 [options] <v0 dts file>...";
+static const char usage_short_opts[] = "" USAGE_COMMON_SHORT_OPTS;
+static struct option const usage_long_opts[] = {
+	USAGE_COMMON_LONG_OPTS
+};
+static const char * const usage_opts_help[] = {
+	USAGE_COMMON_OPTS_HELP
+};
 
 static void convert_file(const char *fname)
 {
@@ -226,10 +230,16 @@ static void convert_file(const char *fname)
 
 int main(int argc, char *argv[])
 {
+	int opt;
 	int i;
 
+	while ((opt = util_getopt_long()) != EOF) {
+		switch (opt) {
+		case_USAGE_COMMON_FLAGS
+		}
+	}
 	if (argc < 2)
-		usage();
+		long_usage("missing filename");
 
 	for (i = 1; i < argc; i++) {
 		fprintf(stderr, "Converting %s from dts v0 to dts v1\n", argv[i]);
diff --git a/dtc.c b/dtc.c
index e4e1b84..d0a1f2d 100644
--- a/dtc.c
+++ b/dtc.c
@@ -47,55 +47,60 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
 		fill_fullpaths(child, tree->fullpath);
 }
 
-static void  __attribute__ ((noreturn)) usage(void)
-{
-	fprintf(stderr, "Usage:\n");
-	fprintf(stderr, "\tdtc [options] <input file>\n");
-	fprintf(stderr, "\nOptions:\n");
-	fprintf(stderr, "\t-h\n");
-	fprintf(stderr, "\t\tThis help text\n");
-	fprintf(stderr, "\t-q\n");
-	fprintf(stderr, "\t\tQuiet: -q suppress warnings, -qq errors, -qqq all\n");
-	fprintf(stderr, "\t-I <input format>\n");
-	fprintf(stderr, "\t\tInput formats are:\n");
-	fprintf(stderr, "\t\t\tdts - device tree source text\n");
-	fprintf(stderr, "\t\t\tdtb - device tree blob\n");
-	fprintf(stderr, "\t\t\tfs - /proc/device-tree style directory\n");
-	fprintf(stderr, "\t-o <output file>\n");
-	fprintf(stderr, "\t-O <output format>\n");
-	fprintf(stderr, "\t\tOutput formats are:\n");
-	fprintf(stderr, "\t\t\tdts - device tree source text\n");
-	fprintf(stderr, "\t\t\tdtb - device tree blob\n");
-	fprintf(stderr, "\t\t\tasm - assembler source\n");
-	fprintf(stderr, "\t-V <output version>\n");
-	fprintf(stderr, "\t\tBlob version to produce, defaults to %d (relevant for dtb\n\t\tand asm output only)\n", DEFAULT_FDT_VERSION);
-	fprintf(stderr, "\t-d <output dependency file>\n");
-	fprintf(stderr, "\t-R <number>\n");
-	fprintf(stderr, "\t\tMake space for <number> reserve map entries (relevant for \n\t\tdtb and asm output only)\n");
-	fprintf(stderr, "\t-S <bytes>\n");
-	fprintf(stderr, "\t\tMake the blob at least <bytes> long (extra space)\n");
-	fprintf(stderr, "\t-p <bytes>\n");
-	fprintf(stderr, "\t\tAdd padding to the blob of <bytes> long (extra space)\n");
-	fprintf(stderr, "\t-b <number>\n");
-	fprintf(stderr, "\t\tSet the physical boot cpu\n");
-	fprintf(stderr, "\t-f\n");
-	fprintf(stderr, "\t\tForce - try to produce output even if the input tree has errors\n");
-	fprintf(stderr, "\t-i\n");
-	fprintf(stderr, "\t\tAdd a path to search for include files\n");
-	fprintf(stderr, "\t-s\n");
-	fprintf(stderr, "\t\tSort nodes and properties before outputting (only useful for\n\t\tcomparing trees)\n");
-	fprintf(stderr, "\t-v\n");
-	fprintf(stderr, "\t\tPrint DTC version and exit\n");
-	fprintf(stderr, "\t-H <phandle format>\n");
-	fprintf(stderr, "\t\tphandle formats are:\n");
-	fprintf(stderr, "\t\t\tlegacy - \"linux,phandle\" properties only\n");
-	fprintf(stderr, "\t\t\tepapr - \"phandle\" properties only\n");
-	fprintf(stderr, "\t\t\tboth - Both \"linux,phandle\" and \"phandle\" properties\n");
-	fprintf(stderr, "\t-W [no-]<checkname>\n");
-	fprintf(stderr, "\t-E [no-]<checkname>\n");
-	fprintf(stderr, "\t\t\tenable or disable warnings and errors\n");
-	exit(3);
-}
+/* Usage related data. */
+static const char usage_synopsis[] = "dtc [options] <input file>";
+static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
+static struct option const usage_long_opts[] = {
+	{"quiet",            no_argument, NULL, 'q'},
+	{"in-format",         a_argument, NULL, 'I'},
+	{"out",               a_argument, NULL, 'o'},
+	{"out-format",        a_argument, NULL, 'O'},
+	{"out-version",       a_argument, NULL, 'V'},
+	{"out-dependency",    a_argument, NULL, 'd'},
+	{"reserve",           a_argument, NULL, 'R'},
+	{"space",             a_argument, NULL, 'S'},
+	{"pad",               a_argument, NULL, 'p'},
+	{"boot-cpu",          a_argument, NULL, 'b'},
+	{"force",            no_argument, NULL, 'f'},
+	{"include",           a_argument, NULL, 'i'},
+	{"sort",             no_argument, NULL, 's'},
+	{"phandle",           a_argument, NULL, 'H'},
+	{"warning",           a_argument, NULL, 'W'},
+	{"error",             a_argument, NULL, 'E'},
+	{"help",             no_argument, NULL, 'h'},
+	{"version",          no_argument, NULL, 'v'},
+	{NULL,               no_argument, NULL, 0x0},
+};
+static const char * const usage_opts_help[] = {
+	"\n\tQuiet: -q suppress warnings, -qq errors, -qqq all",
+	"\n\tInput formats are:\n"
+	 "\t\tdts - device tree source text\n"
+	 "\t\tdtb - device tree blob\n"
+	 "\t\tfs  - /proc/device-tree style directory",
+	"\n\tOutput file",
+	"\n\tOutput formats are:\n"
+	 "\t\tdts - device tree source text\n"
+	 "\t\tdtb - device tree blob\n"
+	 "\t\tasm - assembler source",
+	"\n\tBlob version to produce, defaults to %d (for dtb and asm output)", //, DEFAULT_FDT_VERSION);
+	"\n\tOutput dependency file",
+	"\n\ttMake space for <number> reserve map entries (for dtb and asm output)",
+	"\n\tMake the blob at least <bytes> long (extra space)",
+	"\n\tAdd padding to the blob of <bytes> long (extra space)",
+	"\n\tSet the physical boot cpu",
+	"\n\tTry to produce output even if the input tree has errors",
+	"\n\tAdd a path to search for include files",
+	"\n\tSort nodes and properties before outputting (useful for comparing trees)",
+	"\n\tValid phandle formats are:\n"
+	 "\t\tlegacy - \"linux,phandle\" properties only\n"
+	 "\t\tepapr  - \"phandle\" properties only\n"
+	 "\t\tboth   - Both \"linux,phandle\" and \"phandle\" properties",
+	"\n\tEnable/disable warnings (prefix with \"no-\")",
+	"\n\tEnable/disable errors (prefix with \"no-\")",
+	"\n\tPrint this help and exit",
+	"\n\tPrint version and exit",
+	NULL,
+};
 
 int main(int argc, char *argv[])
 {
@@ -116,8 +121,7 @@ int main(int argc, char *argv[])
 	minsize    = 0;
 	padsize    = 0;
 
-	while ((opt = getopt(argc, argv, "hI:O:o:V:d:R:S:p:fqb:i:vH:sW:E:"))
-			!= EOF) {
+	while ((opt = util_getopt_long()) != EOF) {
 		switch (opt) {
 		case 'I':
 			inform = optarg;
@@ -182,13 +186,14 @@ int main(int argc, char *argv[])
 			break;
 
 		case 'h':
+			long_usage(NULL);
 		default:
-			usage();
+			long_usage("unknown option");
 		}
 	}
 
 	if (argc > (optind+1))
-		usage();
+		long_usage("missing files");
 	else if (argc < (optind+1))
 		arg = "-";
 	else
diff --git a/fdtget.c b/fdtget.c
index 374bee2..c08efd4 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -277,33 +277,33 @@ static int do_fdtget(struct display_info *disp, const char *filename,
 	return 0;
 }
 
-static const char *usage_msg =
-	"fdtget - read values from device tree\n"
-	"\n"
-	"Each value is printed on a new line.\n\n"
-	"Usage:\n"
+/* Usage related data. */
+static const char usage_synopsis[] =
+	"read values from device tree\n"
 	"	fdtget <options> <dt file> [<node> <property>]...\n"
 	"	fdtget -p <options> <dt file> [<node> ]...\n"
-	"Options:\n"
-	"\t-t <type>\tType of data\n"
-	"\t-p\t\tList properties for each node\n"
-	"\t-l\t\tList subnodes for each node\n"
-	"\t-d\t\tDefault value to display when the property is "
-			"missing\n"
-	"\t-h\t\tPrint this help\n\n"
+	"\n"
+	"Each value is printed on a new line.\n"
 	USAGE_TYPE_MSG;
-
-static void usage(const char *msg)
-{
-	if (msg)
-		fprintf(stderr, "Error: %s\n\n", msg);
-
-	fprintf(stderr, "%s", usage_msg);
-	exit(2);
-}
+static const char usage_short_opts[] = "t:pld:" USAGE_COMMON_SHORT_OPTS;
+static struct option const usage_long_opts[] = {
+	{"type",              a_argument, NULL, 't'},
+	{"properties",       no_argument, NULL, 'p'},
+	{"list",             no_argument, NULL, 'l'},
+	{"default",           a_argument, NULL, 'd'},
+	USAGE_COMMON_LONG_OPTS,
+};
+static const char * const usage_opts_help[] = {
+	"Type of data",
+	"List properties for each node",
+	"List subnodes for each node",
+	"Default value to display when the property is missing",
+	USAGE_COMMON_OPTS_HELP
+};
 
 int main(int argc, char *argv[])
 {
+	int opt;
 	char *filename = NULL;
 	struct display_info disp;
 	int args_per_step = 2;
@@ -312,20 +312,14 @@ int main(int argc, char *argv[])
 	memset(&disp, '\0', sizeof(disp));
 	disp.size = -1;
 	disp.mode = MODE_SHOW_VALUE;
-	for (;;) {
-		int c = getopt(argc, argv, "d:hlpt:");
-		if (c == -1)
-			break;
-
-		switch (c) {
-		case 'h':
-		case '?':
-			usage(NULL);
+	while ((opt = util_getopt_long()) != EOF) {
+		switch (opt) {
+		case_USAGE_COMMON_FLAGS
 
 		case 't':
 			if (utilfdt_decode_type(optarg, &disp.type,
 					&disp.size))
-				usage("Invalid type string");
+				long_usage("invalid type string");
 			break;
 
 		case 'p':
@@ -347,7 +341,7 @@ int main(int argc, char *argv[])
 	if (optind < argc)
 		filename = argv[optind++];
 	if (!filename)
-		usage("Missing filename");
+		long_usage("missing filename");
 
 	argv += optind;
 	argc -= optind;
@@ -358,7 +352,7 @@ int main(int argc, char *argv[])
 
 	/* Check for node, property arguments */
 	if (args_per_step == 2 && (argc % 2))
-		usage("Must have an even number of arguments");
+		long_usage("must have an even number of arguments");
 
 	if (do_fdtget(&disp, filename, argv, argc, args_per_step))
 		return 1;
diff --git a/fdtput.c b/fdtput.c
index 572d6da..1be326a 100644
--- a/fdtput.c
+++ b/fdtput.c
@@ -272,44 +272,40 @@ static int do_fdtput(struct display_info *disp, const char *filename,
 	return ret;
 }
 
-static const char *usage_msg =
-	"fdtput - write a property value to a device tree\n"
-	"\n"
-	"The command line arguments are joined together into a single value.\n"
-	"\n"
-	"Usage:\n"
+/* Usage related data. */
+static const char usage_synopsis[] =
+	"write a property value to a device tree\n"
 	"	fdtput <options> <dt file> <node> <property> [<value>...]\n"
 	"	fdtput -c <options> <dt file> [<node>...]\n"
-	"Options:\n"
-	"\t-c\t\tCreate nodes if they don't already exist\n"
-	"\t-p\t\tAutomatically create nodes as needed for the node path\n"
-	"\t-t <type>\tType of data\n"
-	"\t-v\t\tVerbose: display each value decoded from command line\n"
-	"\t-h\t\tPrint this help\n\n"
+	"\n"
+	"The command line arguments are joined together into a single value.\n"
 	USAGE_TYPE_MSG;
-
-static void usage(const char *msg)
-{
-	if (msg)
-		fprintf(stderr, "Error: %s\n\n", msg);
-
-	fprintf(stderr, "%s", usage_msg);
-	exit(2);
-}
+static const char usage_short_opts[] = "cpt:v" USAGE_COMMON_SHORT_OPTS;
+static struct option const usage_long_opts[] = {
+	{"create",           no_argument, NULL, 'c'},
+	{"auto-path",        no_argument, NULL, 'p'},
+	{"type",              a_argument, NULL, 't'},
+	{"verbose",          no_argument, NULL, 'v'},
+	USAGE_COMMON_LONG_OPTS,
+};
+static const char * const usage_opts_help[] = {
+	"Create nodes if they don't already exist",
+	"Automatically create nodes as needed for the node path",
+	"Type of data",
+	"Display each value decoded from command line",
+	USAGE_COMMON_OPTS_HELP
+};
 
 int main(int argc, char *argv[])
 {
+	int opt;
 	struct display_info disp;
 	char *filename = NULL;
 
 	memset(&disp, '\0', sizeof(disp));
 	disp.size = -1;
 	disp.oper = OPER_WRITE_PROP;
-	for (;;) {
-		int c = getopt(argc, argv, "chpt:v");
-		if (c == -1)
-			break;
-
+	while ((opt = util_getopt_long()) != EOF) {
 		/*
 		 * TODO: add options to:
 		 * - delete property
@@ -319,20 +315,19 @@ int main(int argc, char *argv[])
 		 * - set amount of free space when writing
 		 * - expand fdt if value doesn't fit
 		 */
-		switch (c) {
+		switch (opt) {
+		case_USAGE_COMMON_FLAGS
+
 		case 'c':
 			disp.oper = OPER_CREATE_NODE;
 			break;
-		case 'h':
-		case '?':
-			usage(NULL);
 		case 'p':
 			disp.auto_path = 1;
 			break;
 		case 't':
 			if (utilfdt_decode_type(optarg, &disp.type,
 					&disp.size))
-				usage("Invalid type string");
+				long_usage("invalid type string");
 			break;
 
 		case 'v':
@@ -344,16 +339,16 @@ int main(int argc, char *argv[])
 	if (optind < argc)
 		filename = argv[optind++];
 	if (!filename)
-		usage("Missing filename");
+		long_usage("missing filename");
 
 	argv += optind;
 	argc -= optind;
 
 	if (disp.oper == OPER_WRITE_PROP) {
 		if (argc < 1)
-			usage("Missing node");
+			long_usage("missing node");
 		if (argc < 2)
-			usage("Missing property");
+			long_usage("missing property");
 	}
 
 	if (do_fdtput(&disp, filename, argv, argc))
diff --git a/util.h b/util.h
index f9bc777..3d8c0a9 100644
--- a/util.h
+++ b/util.h
@@ -153,7 +153,7 @@ int utilfdt_decode_type(const char *fmt, int *type, int *size);
 #define USAGE_TYPE_MSG \
 	"<type>\ts=string, i=int, u=unsigned, x=hex\n" \
 	"\tOptional modifier prefix:\n" \
-	"\t\thh or b=byte, h=2 byte, l=4 byte (default)\n";
+	"\t\thh or b=byte, h=2 byte, l=4 byte (default)";
 
 /**
  * Print property data in a readable format to stdout
-- 
1.8.1.2

[PATCH 08/10] util: drop "long" from usage helpers

[Mike Frysinger]

Now that all utils have converted to the new usage framework, we can
rename to just plain "usage()" and avoid naming conflicts.

Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 convert-dtsv0-lexer.l |  2 +-
 dtc.c                 |  6 +++---
 fdtdump.c             |  2 +-
 fdtget.c              |  6 +++---
 fdtput.c              |  8 ++++----
 util.c                |  6 +++---
 util.h                | 18 +++++++++---------
 7 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/convert-dtsv0-lexer.l b/convert-dtsv0-lexer.l
index e62d27a..8902648 100644
--- a/convert-dtsv0-lexer.l
+++ b/convert-dtsv0-lexer.l
@@ -239,7 +239,7 @@ int main(int argc, char *argv[])
 		}
 	}
 	if (argc < 2)
-		long_usage("missing filename");
+		usage("missing filename");
 
 	for (i = 1; i < argc; i++) {
 		fprintf(stderr, "Converting %s from dts v0 to dts v1\n", argv[i]);
diff --git a/dtc.c b/dtc.c
index d0a1f2d..e3c9653 100644
--- a/dtc.c
+++ b/dtc.c
@@ -186,14 +186,14 @@ int main(int argc, char *argv[])
 			break;
 
 		case 'h':
-			long_usage(NULL);
+			usage(NULL);
 		default:
-			long_usage("unknown option");
+			usage("unknown option");
 		}
 	}
 
 	if (argc > (optind+1))
-		long_usage("missing files");
+		usage("missing files");
 	else if (argc < (optind+1))
 		arg = "-";
 	else
diff --git a/fdtdump.c b/fdtdump.c
index 7b967aa..2b8c194 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -149,7 +149,7 @@ int main(int argc, char *argv[])
 		}
 	}
 	if (optind != argc - 1)
-		long_usage("missing input filename");
+		usage("missing input filename");
 	file = argv[optind];
 
 	buf = utilfdt_read(file, &len);
diff --git a/fdtget.c b/fdtget.c
index c08efd4..65dd907 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -319,7 +319,7 @@ int main(int argc, char *argv[])
 		case 't':
 			if (utilfdt_decode_type(optarg, &disp.type,
 					&disp.size))
-				long_usage("invalid type string");
+				usage("invalid type string");
 			break;
 
 		case 'p':
@@ -341,7 +341,7 @@ int main(int argc, char *argv[])
 	if (optind < argc)
 		filename = argv[optind++];
 	if (!filename)
-		long_usage("missing filename");
+		usage("missing filename");
 
 	argv += optind;
 	argc -= optind;
@@ -352,7 +352,7 @@ int main(int argc, char *argv[])
 
 	/* Check for node, property arguments */
 	if (args_per_step == 2 && (argc % 2))
-		long_usage("must have an even number of arguments");
+		usage("must have an even number of arguments");
 
 	if (do_fdtget(&disp, filename, argv, argc, args_per_step))
 		return 1;
diff --git a/fdtput.c b/fdtput.c
index 1be326a..bdf9d95 100644
--- a/fdtput.c
+++ b/fdtput.c
@@ -327,7 +327,7 @@ int main(int argc, char *argv[])
 		case 't':
 			if (utilfdt_decode_type(optarg, &disp.type,
 					&disp.size))
-				long_usage("invalid type string");
+				usage("invalid type string");
 			break;
 
 		case 'v':
@@ -339,16 +339,16 @@ int main(int argc, char *argv[])
 	if (optind < argc)
 		filename = argv[optind++];
 	if (!filename)
-		long_usage("missing filename");
+		usage("missing filename");
 
 	argv += optind;
 	argc -= optind;
 
 	if (disp.oper == OPER_WRITE_PROP) {
 		if (argc < 1)
-			long_usage("missing node");
+			usage("missing node");
 		if (argc < 2)
-			long_usage("missing property");
+			usage("missing property");
 	}
 
 	if (do_fdtput(&disp, filename, argv, argc))
diff --git a/util.c b/util.c
index 8d427e4..6ab114b 100644
--- a/util.c
+++ b/util.c
@@ -382,9 +382,9 @@ void util_version(void)
 	exit(0);
 }
 
-void util_long_usage(const char *errmsg, const char *synopsis,
-		     const char *short_opts, struct option const long_opts[],
-		     const char * const opts_help[])
+void util_usage(const char *errmsg, const char *synopsis,
+		const char *short_opts, struct option const long_opts[],
+		const char * const opts_help[])
 {
 	FILE *fp = errmsg ? stderr : stdout;
 	const char a_arg[] = "<arg>";
diff --git a/util.h b/util.h
index 3d8c0a9..3f48664 100644
--- a/util.h
+++ b/util.h
@@ -178,7 +178,7 @@ void util_version(void) __attribute__((noreturn));
  * Show usage and exit
  *
  * This helps standardize the output of various utils.  You most likely want
- * to use the long_usage() helper below rather than call this.
+ * to use the usage() helper below rather than call this.
  *
  * @param errmsg	If non-NULL, an error message to display
  * @param synopsis	The initial example usage text (and possible examples)
@@ -186,9 +186,9 @@ void util_version(void) __attribute__((noreturn));
  * @param long_opts	The structure of long options
  * @param opts_help	An array of help strings (should align with long_opts)
  */
-void util_long_usage(const char *errmsg, const char *synopsis,
-		     const char *short_opts, struct option const long_opts[],
-		     const char * const opts_help[]) __attribute__((noreturn));
+void util_usage(const char *errmsg, const char *synopsis,
+		const char *short_opts, struct option const long_opts[],
+		const char * const opts_help[]) __attribute__((noreturn));
 
 /**
  * Show usage and exit
@@ -198,9 +198,9 @@ void util_long_usage(const char *errmsg, const char *synopsis,
  *
  * @param errmsg	If non-NULL, an error message to display
  */
-#define long_usage(errmsg) \
-	util_long_usage(errmsg, usage_synopsis, usage_short_opts, \
-			usage_long_opts, usage_opts_help)
+#define usage(errmsg) \
+	util_usage(errmsg, usage_synopsis, usage_short_opts, \
+		   usage_long_opts, usage_opts_help)
 
 /**
  * Call getopt_long() with standard options
@@ -230,8 +230,8 @@ void util_long_usage(const char *errmsg, const char *synopsis,
 
 /* Helper for getopt case statements */
 #define case_USAGE_COMMON_FLAGS \
-	case 'h': long_usage(NULL); \
+	case 'h': usage(NULL); \
 	case 'V': util_version(); \
-	case '?': long_usage("unknown option");
+	case '?': usage("unknown option");
 
 #endif /* _UTIL_H */
-- 
1.8.1.2

[PATCH 09/10] util: add common ARRAY_SIZE define

[Mike Frysinger]

I want to use this in more places, so put it in util.h rather than
copying & pasting it into another file.

Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 convert-dtsv0-lexer.l | 2 --
 dtc.h                 | 1 -
 util.h                | 2 ++
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/convert-dtsv0-lexer.l b/convert-dtsv0-lexer.l
index 8902648..548e719 100644
--- a/convert-dtsv0-lexer.l
+++ b/convert-dtsv0-lexer.l
@@ -50,8 +50,6 @@ static int saw_hyphen; /* = 0 */
 static unsigned long long last_val;
 static char *last_name; /* = NULL */
 
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
-
 const struct {
 	const char *pattern;
 	int obase, width;
diff --git a/dtc.h b/dtc.h
index 3e42a07..264a20c 100644
--- a/dtc.h
+++ b/dtc.h
@@ -66,7 +66,6 @@ typedef uint32_t cell_t;
 #define strneq(a, b, n)	(strncmp((a), (b), (n)) == 0)
 
 #define ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 
 /* Data blobs */
 enum markertype {
diff --git a/util.h b/util.h
index 3f48664..958825d 100644
--- a/util.h
+++ b/util.h
@@ -24,6 +24,8 @@
  *                                                                   USA
  */
 
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
 static inline void __attribute__((noreturn)) die(const char *str, ...)
 {
 	va_list ap;
-- 
1.8.1.2

Re: [PATCH 09/10] util: add common ARRAY_SIZE define

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 544 bytes --]

On Wed, Apr 10, 2013 at 02:29:14PM -0400, Mike Frysinger wrote:
> I want to use this in more places, so put it in util.h rather than
> copying & pasting it into another file.
> 
> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>

Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 10/10] fdtdump: add a debug mode

[Mike Frysinger]

When hacking raw fdt files, it's useful to know the actual offsets into
the file each node appears.  Add a --debug mode that includes this.

Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 fdtdump.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/fdtdump.c b/fdtdump.c
index 2b8c194..2c8bff6 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -19,8 +19,29 @@
 #define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
 #define GET_CELL(p)	(p += 4, *((const uint32_t *)(p-4)))
 
-static void dump_blob(void *blob)
+static const char *tagname(uint32_t tag)
 {
+	static const char * const names[] = {
+#define TN(t) [t] #t
+		TN(FDT_BEGIN_NODE),
+		TN(FDT_END_NODE),
+		TN(FDT_PROP),
+		TN(FDT_NOP),
+		TN(FDT_END),
+#undef TN
+	};
+	if (tag < ARRAY_SIZE(names))
+		if (names[tag])
+			return names[tag];
+	return "???";
+}
+
+#define dprintf(fmt, args...) \
+	do { if (debug) printf("// " fmt, ## args); } while (0)
+
+static void dump_blob(void *blob, bool debug)
+{
+	uintptr_t blob_off = (uintptr_t)blob;
 	struct fdt_header *bph = blob;
 	uint32_t off_mem_rsvmap = fdt32_to_cpu(bph->off_mem_rsvmap);
 	uint32_t off_dt = fdt32_to_cpu(bph->off_dt_struct);
@@ -74,7 +95,8 @@ static void dump_blob(void *blob)
 	p = p_struct;
 	while ((tag = fdt32_to_cpu(GET_CELL(p))) != FDT_END) {
 
-		/* printf("tag: 0x%08x (%d)\n", tag, p - p_struct); */
+		dprintf("%04zx: tag: 0x%08x (%s)\n",
+		        (uintptr_t)p - blob_off - 4, tag, tagname(tag));
 
 		if (tag == FDT_BEGIN_NODE) {
 			s = p;
@@ -113,6 +135,8 @@ static void dump_blob(void *blob)
 
 		p = PALIGN(p + sz, 4);
 
+		dprintf("%04zx: string: %s\n", (uintptr_t)s - blob_off, s);
+		dprintf("%04zx: value\n", (uintptr_t)t - blob_off);
 		printf("%*s%s", depth * shift, "", s);
 		utilfdt_print_data(t, sz);
 		printf(";\n");
@@ -121,8 +145,9 @@ static void dump_blob(void *blob)
 
 /* Usage related data. */
 static const char usage_synopsis[] = "fdtdump [options] <file>";
-static const char usage_short_opts[] = "s" USAGE_COMMON_SHORT_OPTS;
+static const char usage_short_opts[] = "ds" USAGE_COMMON_SHORT_OPTS;
 static struct option const usage_long_opts[] = {
+	{"debug",            no_argument, NULL, 'd'},
 	{"scan",             no_argument, NULL, 's'},
 	USAGE_COMMON_LONG_OPTS
 };
@@ -136,6 +161,7 @@ int main(int argc, char *argv[])
 	int opt;
 	const char *file;
 	char *buf;
+	bool debug = false;
 	bool scan = false;
 	off_t len;
 
@@ -143,6 +169,9 @@ int main(int argc, char *argv[])
 		switch (opt) {
 		case_USAGE_COMMON_FLAGS
 
+		case 'd':
+			debug = true;
+			break;
 		case 's':
 			scan = true;
 			break;
@@ -179,6 +208,9 @@ int main(int argc, char *argv[])
 				    fdt_off_dt_struct(p) < this_len &&
 					fdt_off_dt_strings(p) < this_len)
 					break;
+				if (debug)
+					printf("%s: skipping fdt magic at offset %#zx\n",
+						file, p - buf);
 			}
 			++p;
 		}
@@ -188,7 +220,7 @@ int main(int argc, char *argv[])
 		buf = p;
 	}
 
-	dump_blob(buf);
+	dump_blob(buf, debug);
 
 	return 0;
 }
-- 
1.8.1.2

Re: [PATCH 10/10] fdtdump: add a debug mode

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 4236 bytes --]

On Wed, Apr 10, 2013 at 02:29:15PM -0400, Mike Frysinger wrote:
> When hacking raw fdt files, it's useful to know the actual offsets into
> the file each node appears.  Add a --debug mode that includes this.

Neat idea.  Few comments on implementation below.

> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
> ---
>  fdtdump.c | 40 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/fdtdump.c b/fdtdump.c
> index 2b8c194..2c8bff6 100644
> --- a/fdtdump.c
> +++ b/fdtdump.c
> @@ -19,8 +19,29 @@
>  #define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
>  #define GET_CELL(p)	(p += 4, *((const uint32_t *)(p-4)))
>  
> -static void dump_blob(void *blob)
> +static const char *tagname(uint32_t tag)
>  {
> +	static const char * const names[] = {
> +#define TN(t) [t] #t
> +		TN(FDT_BEGIN_NODE),
> +		TN(FDT_END_NODE),
> +		TN(FDT_PROP),
> +		TN(FDT_NOP),
> +		TN(FDT_END),
> +#undef TN
> +	};
> +	if (tag < ARRAY_SIZE(names))
> +		if (names[tag])
> +			return names[tag];
> +	return "???";

Better to return NULL here, I think.  That way a caller can easily
check and print the numeric value instead.

Or you could construct a string with the number.  It would leak
memory, obviously, but in a short-run program like this, it doesn't
really matter.

> +}
> +
> +#define dprintf(fmt, args...) \
> +	do { if (debug) printf("// " fmt, ## args); } while (0)

I'd prefer a different function (well, macro) name.  I tend to use
"dprintf" for debug print functions in the sense of debugging for the
program they're in, whereas in this case it's for the debug mode which
is (primarily) for debugging other programs.

> +static void dump_blob(void *blob, bool debug)
> +{
> +	uintptr_t blob_off = (uintptr_t)blob;
>  	struct fdt_header *bph = blob;
>  	uint32_t off_mem_rsvmap = fdt32_to_cpu(bph->off_mem_rsvmap);
>  	uint32_t off_dt = fdt32_to_cpu(bph->off_dt_struct);
> @@ -74,7 +95,8 @@ static void dump_blob(void *blob)
>  	p = p_struct;
>  	while ((tag = fdt32_to_cpu(GET_CELL(p))) != FDT_END) {
>  
> -		/* printf("tag: 0x%08x (%d)\n", tag, p - p_struct); */
> +		dprintf("%04zx: tag: 0x%08x (%s)\n",
> +		        (uintptr_t)p - blob_off - 4, tag, tagname(tag));
>  
>  		if (tag == FDT_BEGIN_NODE) {
>  			s = p;
> @@ -113,6 +135,8 @@ static void dump_blob(void *blob)
>  
>  		p = PALIGN(p + sz, 4);
>  
> +		dprintf("%04zx: string: %s\n", (uintptr_t)s - blob_off, s);
> +		dprintf("%04zx: value\n", (uintptr_t)t - blob_off);
>  		printf("%*s%s", depth * shift, "", s);
>  		utilfdt_print_data(t, sz);
>  		printf(";\n");
> @@ -121,8 +145,9 @@ static void dump_blob(void *blob)
>  
>  /* Usage related data. */
>  static const char usage_synopsis[] = "fdtdump [options] <file>";
> -static const char usage_short_opts[] = "s" USAGE_COMMON_SHORT_OPTS;
> +static const char usage_short_opts[] = "ds" USAGE_COMMON_SHORT_OPTS;
>  static struct option const usage_long_opts[] = {
> +	{"debug",            no_argument, NULL, 'd'},
>  	{"scan",             no_argument, NULL, 's'},
>  	USAGE_COMMON_LONG_OPTS
>  };
> @@ -136,6 +161,7 @@ int main(int argc, char *argv[])
>  	int opt;
>  	const char *file;
>  	char *buf;
> +	bool debug = false;
>  	bool scan = false;
>  	off_t len;
>  
> @@ -143,6 +169,9 @@ int main(int argc, char *argv[])
>  		switch (opt) {
>  		case_USAGE_COMMON_FLAGS
>  
> +		case 'd':
> +			debug = true;
> +			break;
>  		case 's':
>  			scan = true;
>  			break;
> @@ -179,6 +208,9 @@ int main(int argc, char *argv[])
>  				    fdt_off_dt_struct(p) < this_len &&
>  					fdt_off_dt_strings(p) < this_len)
>  					break;
> +				if (debug)
> +					printf("%s: skipping fdt magic at offset %#zx\n",
> +						file, p - buf);
>  			}
>  			++p;
>  		}
> @@ -188,7 +220,7 @@ int main(int argc, char *argv[])
>  		buf = p;
>  	}
>  
> -	dump_blob(buf);
> +	dump_blob(buf, debug);
>  
>  	return 0;
>  }

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 10/10] fdtdump: add a debug mode

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 1621 bytes --]

On Monday 15 April 2013 01:12:06 David Gibson wrote:
> On Wed, Apr 10, 2013 at 02:29:15PM -0400, Mike Frysinger wrote:
> > -static void dump_blob(void *blob)
> > +static const char *tagname(uint32_t tag)
> >  {
> > +	static const char * const names[] = {
> > +#define TN(t) [t] #t
> > +		TN(FDT_BEGIN_NODE),
> > +		TN(FDT_END_NODE),
> > +		TN(FDT_PROP),
> > +		TN(FDT_NOP),
> > +		TN(FDT_END),
> > +#undef TN
> > +	};
> > +	if (tag < ARRAY_SIZE(names))
> > +		if (names[tag])
> > +			return names[tag];
> > +	return "???";
> 
> Better to return NULL here, I think.  That way a caller can easily
> check and print the numeric value instead.
> 
> Or you could construct a string with the number.  It would leak
> memory, obviously, but in a short-run program like this, it doesn't
> really matter.

if this were a general func (like in util.c or something), i'd agree with you.  
but the only consumer also prints out the #, so it'd be redundant:
        dprintf("%04zx: tag: 0x%08x (%s)\n",
                (uintptr_t)p - blob_off - 4, tag, tagname(tag));

the output looks like:
	// 350c: tag: 0x00000003 (FDT_PROP)

if it was an unknown value, it'd look like:
	// 350c: tag: 0x0000a123 (???)

> > +#define dprintf(fmt, args...) \
> > +	do { if (debug) printf("// " fmt, ## args); } while (0)
> 
> I'd prefer a different function (well, macro) name.  I tend to use
> "dprintf" for debug print functions in the sense of debugging for the
> program they're in, whereas in this case it's for the debug mode which
> is (primarily) for debugging other programs.

dumpf() ?
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 10/10] fdtdump: add a debug mode

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 2068 bytes --]

On Mon, Apr 15, 2013 at 07:35:55PM -0400, Mike Frysinger wrote:
> On Monday 15 April 2013 01:12:06 David Gibson wrote:
> > On Wed, Apr 10, 2013 at 02:29:15PM -0400, Mike Frysinger wrote:
> > > -static void dump_blob(void *blob)
> > > +static const char *tagname(uint32_t tag)
> > >  {
> > > +	static const char * const names[] = {
> > > +#define TN(t) [t] #t
> > > +		TN(FDT_BEGIN_NODE),
> > > +		TN(FDT_END_NODE),
> > > +		TN(FDT_PROP),
> > > +		TN(FDT_NOP),
> > > +		TN(FDT_END),
> > > +#undef TN
> > > +	};
> > > +	if (tag < ARRAY_SIZE(names))
> > > +		if (names[tag])
> > > +			return names[tag];
> > > +	return "???";
> > 
> > Better to return NULL here, I think.  That way a caller can easily
> > check and print the numeric value instead.
> > 
> > Or you could construct a string with the number.  It would leak
> > memory, obviously, but in a short-run program like this, it doesn't
> > really matter.
> 
> if this were a general func (like in util.c or something), i'd agree with you.  
> but the only consumer also prints out the #, so it'd be redundant:
>         dprintf("%04zx: tag: 0x%08x (%s)\n",
>                 (uintptr_t)p - blob_off - 4, tag, tagname(tag));
> 
> the output looks like:
> 	// 350c: tag: 0x00000003 (FDT_PROP)
> 
> if it was an unknown value, it'd look like:
> 	// 350c: tag: 0x0000a123 (???)

Ah, good point, fair enough.

> > > +#define dprintf(fmt, args...) \
> > > +	do { if (debug) printf("// " fmt, ## args); } while (0)
> > 
> > I'd prefer a different function (well, macro) name.  I tend to use
> > "dprintf" for debug print functions in the sense of debugging for the
> > program they're in, whereas in this case it's for the debug mode which
> > is (primarily) for debugging other programs.
> 
> dumpf() ?

Not great, but nothing better springs to mind, so yeah, that'll do.

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 00/10 v3] usage()/--help clean up & unification, and extend fdtdump

[Mike Frysinger]

Each utility currently open codes how they process options and
implement usage().  This leads to inconsistent behavior and output
which sucks.  A bunch also get common things wrong (like sending
--help to stderr).

Rather than go through them one by one and fix their bugs, start
a new mini framework in the util.[ch] code base.  This allows the
utils to be much more data driven in their approach to options --
they declare certain structures with the right format, and then
call usage() as needed.  The new util code takes care of the rest.

I also snuck in a few minor fixes in this patch series that I
noticed as I was hacking on things.  Really I just wanted --scan
and --debug options in the fdtdump tool :).

I've made sure each patch builds & passes `make check` by itself.
So things should be git bisectable.

For the v3 series:
 - add dedicated _len read helpers rather than rework existing ones
 - rename dprintf() to dumpf() in fdtdump

Mike Frysinger (10):
  utilfdt_read_err: use xmalloc funcs
  utilfdt_read: pass back up the length of data read
  die: constify format string arg
  util_version: new helper for displaying version info
  fdtdump: make usage a bit more friendly
  fdtdump: add a --scan option
  dtc/fdt{get,put}/convert-dtsv0-lexer: convert to new usage helpers
  util: drop "long" from usage helpers
  util: add common ARRAY_SIZE define
  fdtdump: add a debug mode

 convert-dtsv0-lexer.l |  24 +++++++----
 dtc.c                 | 116 +++++++++++++++++++++++++-------------------------
 dtc.h                 |   1 -
 fdtdump.c             | 110 ++++++++++++++++++++++++++++++++++++++++++-----
 fdtget.c              |  60 ++++++++++++--------------
 fdtput.c              |  63 +++++++++++++--------------
 util.c                |  84 +++++++++++++++++++++++++++++++++---
 util.h                |  85 +++++++++++++++++++++++++++++++++++-
 8 files changed, 393 insertions(+), 150 deletions(-)

-- 
1.8.1.2

[PATCH 07/10] dtc/fdt{get, put}/convert-dtsv0-lexer: convert to new usage helpers

[Mike Frysinger]

This helps standardize the flag processing and the usage screens.

Only lightly tested; would be great if someone who uses these utils
could double check.

Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 convert-dtsv0-lexer.l |  22 +++++++---
 dtc.c                 | 111 ++++++++++++++++++++++++++------------------------
 fdtget.c              |  60 ++++++++++++---------------
 fdtput.c              |  63 +++++++++++++---------------
 util.h                |   2 +-
 5 files changed, 131 insertions(+), 127 deletions(-)

diff --git a/convert-dtsv0-lexer.l b/convert-dtsv0-lexer.l
index 89d540a..e62d27a 100644
--- a/convert-dtsv0-lexer.l
+++ b/convert-dtsv0-lexer.l
@@ -194,11 +194,15 @@ const struct {
 		}
 
 %%
-static void usage(void)
-{
-	fprintf(stderr, "convert-dtsv0 <v0 dts file>...\n");
-	exit(3);
-}
+/* Usage related data. */
+static const char usage_synopsis[] = "convert-dtsv0 [options] <v0 dts file>...";
+static const char usage_short_opts[] = "" USAGE_COMMON_SHORT_OPTS;
+static struct option const usage_long_opts[] = {
+	USAGE_COMMON_LONG_OPTS
+};
+static const char * const usage_opts_help[] = {
+	USAGE_COMMON_OPTS_HELP
+};
 
 static void convert_file(const char *fname)
 {
@@ -226,10 +230,16 @@ static void convert_file(const char *fname)
 
 int main(int argc, char *argv[])
 {
+	int opt;
 	int i;
 
+	while ((opt = util_getopt_long()) != EOF) {
+		switch (opt) {
+		case_USAGE_COMMON_FLAGS
+		}
+	}
 	if (argc < 2)
-		usage();
+		long_usage("missing filename");
 
 	for (i = 1; i < argc; i++) {
 		fprintf(stderr, "Converting %s from dts v0 to dts v1\n", argv[i]);
diff --git a/dtc.c b/dtc.c
index e4e1b84..d0a1f2d 100644
--- a/dtc.c
+++ b/dtc.c
@@ -47,55 +47,60 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
 		fill_fullpaths(child, tree->fullpath);
 }
 
-static void  __attribute__ ((noreturn)) usage(void)
-{
-	fprintf(stderr, "Usage:\n");
-	fprintf(stderr, "\tdtc [options] <input file>\n");
-	fprintf(stderr, "\nOptions:\n");
-	fprintf(stderr, "\t-h\n");
-	fprintf(stderr, "\t\tThis help text\n");
-	fprintf(stderr, "\t-q\n");
-	fprintf(stderr, "\t\tQuiet: -q suppress warnings, -qq errors, -qqq all\n");
-	fprintf(stderr, "\t-I <input format>\n");
-	fprintf(stderr, "\t\tInput formats are:\n");
-	fprintf(stderr, "\t\t\tdts - device tree source text\n");
-	fprintf(stderr, "\t\t\tdtb - device tree blob\n");
-	fprintf(stderr, "\t\t\tfs - /proc/device-tree style directory\n");
-	fprintf(stderr, "\t-o <output file>\n");
-	fprintf(stderr, "\t-O <output format>\n");
-	fprintf(stderr, "\t\tOutput formats are:\n");
-	fprintf(stderr, "\t\t\tdts - device tree source text\n");
-	fprintf(stderr, "\t\t\tdtb - device tree blob\n");
-	fprintf(stderr, "\t\t\tasm - assembler source\n");
-	fprintf(stderr, "\t-V <output version>\n");
-	fprintf(stderr, "\t\tBlob version to produce, defaults to %d (relevant for dtb\n\t\tand asm output only)\n", DEFAULT_FDT_VERSION);
-	fprintf(stderr, "\t-d <output dependency file>\n");
-	fprintf(stderr, "\t-R <number>\n");
-	fprintf(stderr, "\t\tMake space for <number> reserve map entries (relevant for \n\t\tdtb and asm output only)\n");
-	fprintf(stderr, "\t-S <bytes>\n");
-	fprintf(stderr, "\t\tMake the blob at least <bytes> long (extra space)\n");
-	fprintf(stderr, "\t-p <bytes>\n");
-	fprintf(stderr, "\t\tAdd padding to the blob of <bytes> long (extra space)\n");
-	fprintf(stderr, "\t-b <number>\n");
-	fprintf(stderr, "\t\tSet the physical boot cpu\n");
-	fprintf(stderr, "\t-f\n");
-	fprintf(stderr, "\t\tForce - try to produce output even if the input tree has errors\n");
-	fprintf(stderr, "\t-i\n");
-	fprintf(stderr, "\t\tAdd a path to search for include files\n");
-	fprintf(stderr, "\t-s\n");
-	fprintf(stderr, "\t\tSort nodes and properties before outputting (only useful for\n\t\tcomparing trees)\n");
-	fprintf(stderr, "\t-v\n");
-	fprintf(stderr, "\t\tPrint DTC version and exit\n");
-	fprintf(stderr, "\t-H <phandle format>\n");
-	fprintf(stderr, "\t\tphandle formats are:\n");
-	fprintf(stderr, "\t\t\tlegacy - \"linux,phandle\" properties only\n");
-	fprintf(stderr, "\t\t\tepapr - \"phandle\" properties only\n");
-	fprintf(stderr, "\t\t\tboth - Both \"linux,phandle\" and \"phandle\" properties\n");
-	fprintf(stderr, "\t-W [no-]<checkname>\n");
-	fprintf(stderr, "\t-E [no-]<checkname>\n");
-	fprintf(stderr, "\t\t\tenable or disable warnings and errors\n");
-	exit(3);
-}
+/* Usage related data. */
+static const char usage_synopsis[] = "dtc [options] <input file>";
+static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
+static struct option const usage_long_opts[] = {
+	{"quiet",            no_argument, NULL, 'q'},
+	{"in-format",         a_argument, NULL, 'I'},
+	{"out",               a_argument, NULL, 'o'},
+	{"out-format",        a_argument, NULL, 'O'},
+	{"out-version",       a_argument, NULL, 'V'},
+	{"out-dependency",    a_argument, NULL, 'd'},
+	{"reserve",           a_argument, NULL, 'R'},
+	{"space",             a_argument, NULL, 'S'},
+	{"pad",               a_argument, NULL, 'p'},
+	{"boot-cpu",          a_argument, NULL, 'b'},
+	{"force",            no_argument, NULL, 'f'},
+	{"include",           a_argument, NULL, 'i'},
+	{"sort",             no_argument, NULL, 's'},
+	{"phandle",           a_argument, NULL, 'H'},
+	{"warning",           a_argument, NULL, 'W'},
+	{"error",             a_argument, NULL, 'E'},
+	{"help",             no_argument, NULL, 'h'},
+	{"version",          no_argument, NULL, 'v'},
+	{NULL,               no_argument, NULL, 0x0},
+};
+static const char * const usage_opts_help[] = {
+	"\n\tQuiet: -q suppress warnings, -qq errors, -qqq all",
+	"\n\tInput formats are:\n"
+	 "\t\tdts - device tree source text\n"
+	 "\t\tdtb - device tree blob\n"
+	 "\t\tfs  - /proc/device-tree style directory",
+	"\n\tOutput file",
+	"\n\tOutput formats are:\n"
+	 "\t\tdts - device tree source text\n"
+	 "\t\tdtb - device tree blob\n"
+	 "\t\tasm - assembler source",
+	"\n\tBlob version to produce, defaults to %d (for dtb and asm output)", //, DEFAULT_FDT_VERSION);
+	"\n\tOutput dependency file",
+	"\n\ttMake space for <number> reserve map entries (for dtb and asm output)",
+	"\n\tMake the blob at least <bytes> long (extra space)",
+	"\n\tAdd padding to the blob of <bytes> long (extra space)",
+	"\n\tSet the physical boot cpu",
+	"\n\tTry to produce output even if the input tree has errors",
+	"\n\tAdd a path to search for include files",
+	"\n\tSort nodes and properties before outputting (useful for comparing trees)",
+	"\n\tValid phandle formats are:\n"
+	 "\t\tlegacy - \"linux,phandle\" properties only\n"
+	 "\t\tepapr  - \"phandle\" properties only\n"
+	 "\t\tboth   - Both \"linux,phandle\" and \"phandle\" properties",
+	"\n\tEnable/disable warnings (prefix with \"no-\")",
+	"\n\tEnable/disable errors (prefix with \"no-\")",
+	"\n\tPrint this help and exit",
+	"\n\tPrint version and exit",
+	NULL,
+};
 
 int main(int argc, char *argv[])
 {
@@ -116,8 +121,7 @@ int main(int argc, char *argv[])
 	minsize    = 0;
 	padsize    = 0;
 
-	while ((opt = getopt(argc, argv, "hI:O:o:V:d:R:S:p:fqb:i:vH:sW:E:"))
-			!= EOF) {
+	while ((opt = util_getopt_long()) != EOF) {
 		switch (opt) {
 		case 'I':
 			inform = optarg;
@@ -182,13 +186,14 @@ int main(int argc, char *argv[])
 			break;
 
 		case 'h':
+			long_usage(NULL);
 		default:
-			usage();
+			long_usage("unknown option");
 		}
 	}
 
 	if (argc > (optind+1))
-		usage();
+		long_usage("missing files");
 	else if (argc < (optind+1))
 		arg = "-";
 	else
diff --git a/fdtget.c b/fdtget.c
index c2fbab2..5008cc1 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -277,33 +277,33 @@ static int do_fdtget(struct display_info *disp, const char *filename,
 	return 0;
 }
 
-static const char *usage_msg =
-	"fdtget - read values from device tree\n"
-	"\n"
-	"Each value is printed on a new line.\n\n"
-	"Usage:\n"
+/* Usage related data. */
+static const char usage_synopsis[] =
+	"read values from device tree\n"
 	"	fdtget <options> <dt file> [<node> <property>]...\n"
 	"	fdtget -p <options> <dt file> [<node> ]...\n"
-	"Options:\n"
-	"\t-t <type>\tType of data\n"
-	"\t-p\t\tList properties for each node\n"
-	"\t-l\t\tList subnodes for each node\n"
-	"\t-d\t\tDefault value to display when the property is "
-			"missing\n"
-	"\t-h\t\tPrint this help\n\n"
+	"\n"
+	"Each value is printed on a new line.\n"
 	USAGE_TYPE_MSG;
-
-static void usage(const char *msg)
-{
-	if (msg)
-		fprintf(stderr, "Error: %s\n\n", msg);
-
-	fprintf(stderr, "%s", usage_msg);
-	exit(2);
-}
+static const char usage_short_opts[] = "t:pld:" USAGE_COMMON_SHORT_OPTS;
+static struct option const usage_long_opts[] = {
+	{"type",              a_argument, NULL, 't'},
+	{"properties",       no_argument, NULL, 'p'},
+	{"list",             no_argument, NULL, 'l'},
+	{"default",           a_argument, NULL, 'd'},
+	USAGE_COMMON_LONG_OPTS,
+};
+static const char * const usage_opts_help[] = {
+	"Type of data",
+	"List properties for each node",
+	"List subnodes for each node",
+	"Default value to display when the property is missing",
+	USAGE_COMMON_OPTS_HELP
+};
 
 int main(int argc, char *argv[])
 {
+	int opt;
 	char *filename = NULL;
 	struct display_info disp;
 	int args_per_step = 2;
@@ -312,20 +312,14 @@ int main(int argc, char *argv[])
 	memset(&disp, '\0', sizeof(disp));
 	disp.size = -1;
 	disp.mode = MODE_SHOW_VALUE;
-	for (;;) {
-		int c = getopt(argc, argv, "d:hlpt:");
-		if (c == -1)
-			break;
-
-		switch (c) {
-		case 'h':
-		case '?':
-			usage(NULL);
+	while ((opt = util_getopt_long()) != EOF) {
+		switch (opt) {
+		case_USAGE_COMMON_FLAGS
 
 		case 't':
 			if (utilfdt_decode_type(optarg, &disp.type,
 					&disp.size))
-				usage("Invalid type string");
+				long_usage("invalid type string");
 			break;
 
 		case 'p':
@@ -347,7 +341,7 @@ int main(int argc, char *argv[])
 	if (optind < argc)
 		filename = argv[optind++];
 	if (!filename)
-		usage("Missing filename");
+		long_usage("missing filename");
 
 	argv += optind;
 	argc -= optind;
@@ -358,7 +352,7 @@ int main(int argc, char *argv[])
 
 	/* Check for node, property arguments */
 	if (args_per_step == 2 && (argc % 2))
-		usage("Must have an even number of arguments");
+		long_usage("must have an even number of arguments");
 
 	if (do_fdtget(&disp, filename, argv, argc, args_per_step))
 		return 1;
diff --git a/fdtput.c b/fdtput.c
index f2197f5..f7bf56e 100644
--- a/fdtput.c
+++ b/fdtput.c
@@ -272,44 +272,40 @@ static int do_fdtput(struct display_info *disp, const char *filename,
 	return ret;
 }
 
-static const char *usage_msg =
-	"fdtput - write a property value to a device tree\n"
-	"\n"
-	"The command line arguments are joined together into a single value.\n"
-	"\n"
-	"Usage:\n"
+/* Usage related data. */
+static const char usage_synopsis[] =
+	"write a property value to a device tree\n"
 	"	fdtput <options> <dt file> <node> <property> [<value>...]\n"
 	"	fdtput -c <options> <dt file> [<node>...]\n"
-	"Options:\n"
-	"\t-c\t\tCreate nodes if they don't already exist\n"
-	"\t-p\t\tAutomatically create nodes as needed for the node path\n"
-	"\t-t <type>\tType of data\n"
-	"\t-v\t\tVerbose: display each value decoded from command line\n"
-	"\t-h\t\tPrint this help\n\n"
+	"\n"
+	"The command line arguments are joined together into a single value.\n"
 	USAGE_TYPE_MSG;
-
-static void usage(const char *msg)
-{
-	if (msg)
-		fprintf(stderr, "Error: %s\n\n", msg);
-
-	fprintf(stderr, "%s", usage_msg);
-	exit(2);
-}
+static const char usage_short_opts[] = "cpt:v" USAGE_COMMON_SHORT_OPTS;
+static struct option const usage_long_opts[] = {
+	{"create",           no_argument, NULL, 'c'},
+	{"auto-path",        no_argument, NULL, 'p'},
+	{"type",              a_argument, NULL, 't'},
+	{"verbose",          no_argument, NULL, 'v'},
+	USAGE_COMMON_LONG_OPTS,
+};
+static const char * const usage_opts_help[] = {
+	"Create nodes if they don't already exist",
+	"Automatically create nodes as needed for the node path",
+	"Type of data",
+	"Display each value decoded from command line",
+	USAGE_COMMON_OPTS_HELP
+};
 
 int main(int argc, char *argv[])
 {
+	int opt;
 	struct display_info disp;
 	char *filename = NULL;
 
 	memset(&disp, '\0', sizeof(disp));
 	disp.size = -1;
 	disp.oper = OPER_WRITE_PROP;
-	for (;;) {
-		int c = getopt(argc, argv, "chpt:v");
-		if (c == -1)
-			break;
-
+	while ((opt = util_getopt_long()) != EOF) {
 		/*
 		 * TODO: add options to:
 		 * - delete property
@@ -319,20 +315,19 @@ int main(int argc, char *argv[])
 		 * - set amount of free space when writing
 		 * - expand fdt if value doesn't fit
 		 */
-		switch (c) {
+		switch (opt) {
+		case_USAGE_COMMON_FLAGS
+
 		case 'c':
 			disp.oper = OPER_CREATE_NODE;
 			break;
-		case 'h':
-		case '?':
-			usage(NULL);
 		case 'p':
 			disp.auto_path = 1;
 			break;
 		case 't':
 			if (utilfdt_decode_type(optarg, &disp.type,
 					&disp.size))
-				usage("Invalid type string");
+				long_usage("invalid type string");
 			break;
 
 		case 'v':
@@ -344,16 +339,16 @@ int main(int argc, char *argv[])
 	if (optind < argc)
 		filename = argv[optind++];
 	if (!filename)
-		usage("Missing filename");
+		long_usage("missing filename");
 
 	argv += optind;
 	argc -= optind;
 
 	if (disp.oper == OPER_WRITE_PROP) {
 		if (argc < 1)
-			usage("Missing node");
+			long_usage("missing node");
 		if (argc < 2)
-			usage("Missing property");
+			long_usage("missing property");
 	}
 
 	if (do_fdtput(&disp, filename, argv, argc))
diff --git a/util.h b/util.h
index 1da3bd3..439b2b3 100644
--- a/util.h
+++ b/util.h
@@ -164,7 +164,7 @@ int utilfdt_decode_type(const char *fmt, int *type, int *size);
 #define USAGE_TYPE_MSG \
 	"<type>\ts=string, i=int, u=unsigned, x=hex\n" \
 	"\tOptional modifier prefix:\n" \
-	"\t\thh or b=byte, h=2 byte, l=4 byte (default)\n";
+	"\t\thh or b=byte, h=2 byte, l=4 byte (default)";
 
 /**
  * Print property data in a readable format to stdout
-- 
1.8.1.2

Re: [PATCH 07/10] dtc/fdt{get, put}/convert-dtsv0-lexer: convert to new usage helpers

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 598 bytes --]

On Mon, Apr 15, 2013 at 10:13:14PM -0400, Mike Frysinger wrote:
> This helps standardize the flag processing and the usage screens.
> 
> Only lightly tested; would be great if someone who uses these utils
> could double check.
> 
> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>

Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss