[PATCH] DTC: Improve options handling

[PATCH] DTC: Improve options handling

[Jerry Van Baren]

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

Hi David, Jon,

Here is another fairly trivial patch for dtc.  It adds -h to get the 
usage message.  It adds -q (-qq/-qqq) to suppress warnings/errors.
It removes the -R option which is advertised but does nothing useful.

Best regards,
gvb

[-- Attachment #2: 0001-Improve-options-handling.txt --]
[-- Type: text/plain, Size: 4119 bytes --]

Subject: [PATCH] Improve options handling

Remove unused -R option
Add -h option for help
Add -q quiet option to reduce or suppress the whining

Signed-off-by: vanbaren@cideas.com <vanbaren@cideas.com>
---
 dtc.c      |   24 ++++++++++++++++--------
 dtc.h      |    5 +++++
 livetree.c |   10 ++++++----
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/dtc.c b/dtc.c
index f15d90f..dc0077b 100644
--- a/dtc.c
+++ b/dtc.c
@@ -81,6 +81,10 @@ static void 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");
@@ -93,7 +97,6 @@ static void usage(void)
 	fprintf(stderr, "\t\t\tasm - assembler source\n");
 	fprintf(stderr, "\t-V <output version>\n");
 	fprintf(stderr, "\t\tBlob version to produce, defaults to 16 (relevant for dtb\n\t\tand asm output only)\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-b <number>\n");
 	fprintf(stderr, "\t\tSet the physical boot cpu\n");
@@ -114,10 +117,11 @@ int main(int argc, char *argv[])
 	FILE *inf = NULL;
 	FILE *outf = NULL;
 	int outversion = 17;
-	int reservenum = 1;
 	int boot_cpuid_phys = 0xfeedbeef;
 
-	while ((opt = getopt(argc, argv, "I:O:o:V:R:fb:")) != EOF) {
+	quiet = 0;
+
+	while ((opt = getopt(argc, argv, "hI:O:o:V:fqb:")) != EOF) {
 		switch (opt) {
 		case 'I':
 			inform = optarg;
@@ -131,15 +135,16 @@ int main(int argc, char *argv[])
 		case 'V':
 			outversion = strtol(optarg, NULL, 0);
 			break;
-		case 'R':
-			reservenum = strtol(optarg, NULL, 0);
-			break;
 		case 'f':
 			force = 1;
 			break;
+		case 'q':
+			quiet++;
+			break;
 		case 'b':
 			boot_cpuid_phys = strtol(optarg, NULL, 0);
 			break;
+		case 'h':
 		default:
 			usage();
 		}
@@ -174,9 +179,12 @@ int main(int argc, char *argv[])
 		die("Couldn't read input tree\n");
 
 	if (! check_device_tree(bi->dt, outversion, boot_cpuid_phys)) {
-		fprintf(stderr, "Input tree has errors\n");
-		if (! force)
+		if ((force) && (quiet < 3))
+			fprintf(stderr, "Input tree has errors, output forced\n");
+		if (! force) {
+			fprintf(stderr, "Input tree has errors, not writing output (use -f to force output)\n");
 			exit(1);
+		}
 	}
 
 	if (streq(outname, "-")) {
diff --git a/dtc.h b/dtc.h
index 8d3964c..e3e2863 100644
--- a/dtc.h
+++ b/dtc.h
@@ -36,6 +36,11 @@
 
 #include "flat_dt.h"
 
+/*
+ * Level of quietness
+ */
+int quiet;
+
 static inline void die(char * str, ...)
 {
 	va_list ap;
diff --git a/livetree.c b/livetree.c
index 84f2f64..f7b6108 100644
--- a/livetree.c
+++ b/livetree.c
@@ -252,8 +252,8 @@ static struct node *get_node_by_phandle(struct node *tree, cell_t phandle)
  * Tree checking functions
  */
 
-#define ERRMSG(...) fprintf(stderr, "ERROR: " __VA_ARGS__)
-#define WARNMSG(...) fprintf(stderr, "Warning: " __VA_ARGS__)
+#define ERRMSG(...) if (quiet < 2) fprintf(stderr, "ERROR: " __VA_ARGS__)
+#define WARNMSG(...) if (quiet < 1) fprintf(stderr, "Warning: " __VA_ARGS__)
 
 static int must_be_one_cell(struct property *prop, struct node *node)
 {
@@ -512,13 +512,15 @@ static int check_cpus(struct node *root, int outversion, int boot_cpuid_phys)
 			char *eptr;
 
 			unitnum = strtol(get_unitname(cpu), &eptr, 16);
-			if (*eptr)
+			if (*eptr) {
 				WARNMSG("%s has bad format unit name %s (should be CPU number\n",
 					cpu->fullpath, get_unitname(cpu));
-			else if (unitnum != propval_cell(prop))
+			}
+			else if (unitnum != propval_cell(prop)) {
 				WARNMSG("%s unit name \"%s\" does not match \"reg\" property <%x>\n",
 				       cpu->fullpath, get_unitname(cpu),
 				       propval_cell(prop));
+			}
 		}
 
 /* 		CHECK_HAVE_ONECELL(cpu, "d-cache-line-size"); */
-- 
1.4.4.4

Re: [PATCH] DTC: Improve options handling

[David Gibson]

On Sat, Mar 17, 2007 at 01:03:37PM -0400, Jerry Van Baren wrote:
> Hi David, Jon,
> 
> Here is another fairly trivial patch for dtc.  It adds -h to get the 
> usage message.  It adds -q (-qq/-qqq) to suppress warnings/errors.
> It removes the -R option which is advertised but does nothing
> useful.

NAK.  -R does do something that I believe is useful.

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

Re: [PATCH] DTC: Improve options handling

[Jerry Van Baren]

David Gibson wrote:
> On Sat, Mar 17, 2007 at 01:03:37PM -0400, Jerry Van Baren wrote:
>> Hi David, Jon,
>>
>> Here is another fairly trivial patch for dtc.  It adds -h to get the 
>> usage message.  It adds -q (-qq/-qqq) to suppress warnings/errors.
>> It removes the -R option which is advertised but does nothing
>> useful.
> 
> NAK.  -R does do something that I believe is useful.

Hi David,

-R sets a local variable reservenum to the specified value.  Nobody uses 
reservenum, hence my conclusion that it does nothing useful.

Best regards,
gvb

Re: [PATCH] DTC: Improve options handling

[David Gibson]

On Sat, Mar 17, 2007 at 08:35:47PM -0400, Jerry Van Baren wrote:
> David Gibson wrote:
> > On Sat, Mar 17, 2007 at 01:03:37PM -0400, Jerry Van Baren wrote:
> >> Hi David, Jon,
> >>
> >> Here is another fairly trivial patch for dtc.  It adds -h to get the 
> >> usage message.  It adds -q (-qq/-qqq) to suppress warnings/errors.
> >> It removes the -R option which is advertised but does nothing
> >> useful.
> > 
> > NAK.  -R does do something that I believe is useful.
> 
> Hi David,
> 
> -R sets a local variable reservenum to the specified value.  Nobody uses 
> reservenum, hence my conclusion that it does nothing useful.

Oops, looks like -R got broken when /memreserve/ was implemented.
It's supposed to add extra, empty, entries into the reserve map for
the benefit of dumb bootloaders that want to add reserve entries
without having to rearrange the blob.

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

Re: [PATCH] DTC: Improve options handling - respin

[Jerry Van Baren]

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

David Gibson wrote:
> On Sat, Mar 17, 2007 at 01:03:37PM -0400, Jerry Van Baren wrote:
>> Hi David, Jon,
>>
>> Here is another fairly trivial patch for dtc.  It adds -h to get the 
>> usage message.  It adds -q (-qq/-qqq) to suppress warnings/errors.
>> It removes the -R option which is advertised but does nothing
>> useful.
> 
> NAK.  -R does do something that I believe is useful.

Hi David, Jon,

I was just seeing if I could get a rise out of you. It worked. ;-)

Attached is a respin of my previous two patches: it adds the -h and -q 
options and #defines the default version but does _not_ delete the -R 
option (left as an exercise for the reader to make it useful :-).

Best regards,
gvb

[-- Attachment #2: 0001-Improve-options-define-default-version.txt --]
[-- Type: text/plain, Size: 4547 bytes --]

Subject: [PATCH] Improve options, #define default version.

Add -h option for help
Add -q quiet option to reduce or suppress the whining
Create #define for the default version value.

Signed-off-by: vanbaren@cideas.com <vanbaren@cideas.com>
---
 dtc.c      |   23 ++++++++++++++++++-----
 dtc.h      |    5 +++++
 flat_dt.h  |    2 ++
 livetree.c |    9 +++++----
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/dtc.c b/dtc.c
index f15d90f..051a68b 100644
--- a/dtc.c
+++ b/dtc.c
@@ -81,6 +81,10 @@ static void 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");
@@ -92,7 +96,7 @@ static void usage(void)
 	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 16 (relevant for dtb\n\t\tand asm output only)\n");
+	fprintf(stderr, "\t\tBlob version to produce, defaults to %d (relevant for dtb\n\t\tand asm output only)\n", OF_DEFAULT_VERSION);
 	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-b <number>\n");
@@ -113,11 +117,13 @@ int main(int argc, char *argv[])
 	int opt;
 	FILE *inf = NULL;
 	FILE *outf = NULL;
-	int outversion = 17;
+	int outversion = OF_DEFAULT_VERSION;
 	int reservenum = 1;
 	int boot_cpuid_phys = 0xfeedbeef;
 
-	while ((opt = getopt(argc, argv, "I:O:o:V:R:fb:")) != EOF) {
+	quiet = 0;
+
+	while ((opt = getopt(argc, argv, "hI:O:o:V:R:fqb:")) != EOF) {
 		switch (opt) {
 		case 'I':
 			inform = optarg;
@@ -137,9 +143,13 @@ int main(int argc, char *argv[])
 		case 'f':
 			force = 1;
 			break;
+		case 'q':
+			quiet++;
+			break;
 		case 'b':
 			boot_cpuid_phys = strtol(optarg, NULL, 0);
 			break;
+		case 'h':
 		default:
 			usage();
 		}
@@ -174,9 +184,12 @@ int main(int argc, char *argv[])
 		die("Couldn't read input tree\n");
 
 	if (! check_device_tree(bi->dt, outversion, boot_cpuid_phys)) {
-		fprintf(stderr, "Input tree has errors\n");
-		if (! force)
+		if ((force) && (quiet < 3))
+			fprintf(stderr, "Input tree has errors, output forced\n");
+		if (! force) {
+			fprintf(stderr, "Input tree has errors, not writing output (use -f to force output)\n");
 			exit(1);
+		}
 	}
 
 	if (streq(outname, "-")) {
diff --git a/dtc.h b/dtc.h
index 8d3964c..e3e2863 100644
--- a/dtc.h
+++ b/dtc.h
@@ -36,6 +36,11 @@
 
 #include "flat_dt.h"
 
+/*
+ * Level of quietness
+ */
+int quiet;
+
 static inline void die(char * str, ...)
 {
 	va_list ap;
diff --git a/flat_dt.h b/flat_dt.h
index d5abd80..a8482b4 100644
--- a/flat_dt.h
+++ b/flat_dt.h
@@ -2,6 +2,8 @@
 #define _FLAT_DT_H_
 
 
+#define OF_DEFAULT_VERSION	17
+
 #define OF_DT_HEADER            0xd00dfeed      /* 4: version, 4: total size */
 
 #define OF_DT_BEGIN_NODE	0x1             /* Start node: full name */
diff --git a/livetree.c b/livetree.c
index 84f2f64..45642dc 100644
--- a/livetree.c
+++ b/livetree.c
@@ -252,8 +252,8 @@ static struct node *get_node_by_phandle(struct node *tree, cell_t phandle)
  * Tree checking functions
  */
 
-#define ERRMSG(...) fprintf(stderr, "ERROR: " __VA_ARGS__)
-#define WARNMSG(...) fprintf(stderr, "Warning: " __VA_ARGS__)
+#define ERRMSG(...) if (quiet < 2) fprintf(stderr, "ERROR: " __VA_ARGS__)
+#define WARNMSG(...) if (quiet < 1) fprintf(stderr, "Warning: " __VA_ARGS__)
 
 static int must_be_one_cell(struct property *prop, struct node *node)
 {
@@ -512,13 +512,14 @@ static int check_cpus(struct node *root, int outversion, int boot_cpuid_phys)
 			char *eptr;
 
 			unitnum = strtol(get_unitname(cpu), &eptr, 16);
-			if (*eptr)
+			if (*eptr) {
 				WARNMSG("%s has bad format unit name %s (should be CPU number\n",
 					cpu->fullpath, get_unitname(cpu));
-			else if (unitnum != propval_cell(prop))
+			} else if (unitnum != propval_cell(prop)) {
 				WARNMSG("%s unit name \"%s\" does not match \"reg\" property <%x>\n",
 				       cpu->fullpath, get_unitname(cpu),
 				       propval_cell(prop));
+			}
 		}
 
 /* 		CHECK_HAVE_ONECELL(cpu, "d-cache-line-size"); */
-- 
1.4.4.4

Re: [PATCH] DTC: Improve options handling - respin

[Jon Loeliger]

So, like, the other day Jerry Van Baren mumbled:
> 
> Hi David, Jon,
> 
> I was just seeing if I could get a rise out of you. It worked. ;-)
> 
> Attached is a respin of my previous two patches: it adds the -h and -q 
> options and #defines the default version but does _not_ delete the -R 
> option (left as an exercise for the reader to make it useful :-).

Please follow the patch layout guideline like this:

    Mail headers with
    From: <"default" author>
    Subject: <patch summary line>

    From: <actual author if different than mail envelope author>

    <detailed log message>

    <sign-off as needed>
    ---

    Notes and commentary

    <actual patch inline text>

As it was, manual patch editing was necessary to dodge
your introductory explanation.

> Subject: [PATCH] Improve options, #define default version.
> 
> Add -h option for help
> Add -q quiet option to reduce or suppress the whining
> Create #define for the default version value.
> 
> Signed-off-by: vanbaren@cideas.com <vanbaren@cideas.com>
> ---

Applied and pushed out!

However, two comments:

> ... but does _not_ delete the -R 
> option (left as an exercise for the reader to make it useful :-).

I'll certainly entertain a patch to fix the reserve space problem. :-)
That could have been my fault, though we both had our fingers in that pie.

> +	fprintf(stderr, "\t-q\n");
> +	fprintf(stderr, "\t\tQuiet: -q suppress warnings, -qq errors, -qqq all\n");

This is a bit misleading.  Sepcifically, -qqq does not currently
suppres "all" messagesm notably	those through yyerror().
An ambitious soul might beat me to fixing that up...


Thanks,
jdl