DTC: Improve options handling

PATCH In review Jerry Van Baren 3 participants 6 messages Mar 17, 2007 – Mar 19, 2007

lore

Share & tools

lore

Jerry Van Baren

author

2007-03-17 17:03 UTC #0 lore raw

PATCH 1/1 DTC: Improve options handling

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

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

---
 dtc.c      |   24 ++++++++++++++++--------
 dtc.h      |    5 +++++
 livetree.c |   10 ++++++----
 3 files changed, 27 insertions(+), 12 deletions(-)

dtc.c +16 -8

index f15d90f..dc0077b 100644
@@ -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, "-")) {

dtc.h +5 -0

index 8d3964c..e3e2863 100644
@@ -36,6 +36,11 @@
 
 #include "flat_dt.h"
 
+/*
+ * Level of quietness
+ */
+int quiet;
+
 static inline void die(char * str, ...)
 {
 	va_list ap;

livetree.c +6 -4

index 84f2f64..f7b6108 100644
@@ -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"); */

Signature · 1 line

1.4.4.4

David Gibson

2007-03-17 23:13 UTC #1 lore raw

Quoted 6 lines · Jerry Van Baren

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.

Signature · 4 lines

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

Jerry Van Baren

author

2007-03-18 00:35 UTC #2 lore raw

Quoted 8 lines · David Gibson

Quoted 5 lines · Jerry Van Baren
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

David Gibson

2007-03-18 01:51 UTC #3 lore raw

Quoted 9 lines · Jerry Van Baren

Quoted 8 lines · David Gibson
Quoted 6 lines · Jerry Van Baren
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.

Quoted 6 lines

Quoted 1 line
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.

Signature · 4 lines

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

Jerry Van Baren

author

2007-03-18 20:49 UTC #4 lore raw

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

In reply to David Gibson

Quoted 8 lines · David Gibson

Quoted 5 lines · Jerry Van Baren
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

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

---
 dtc.c      |   23 ++++++++++++++++++-----
 dtc.h      |    5 +++++
 flat_dt.h  |    2 ++
 livetree.c |    9 +++++----
 4 files changed, 30 insertions(+), 9 deletions(-)

dtc.c +18 -5

index f15d90f..051a68b 100644
@@ -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, "-")) {

dtc.h +5 -0

index 8d3964c..e3e2863 100644
@@ -36,6 +36,11 @@
 
 #include "flat_dt.h"
 
+/*
+ * Level of quietness
+ */
+int quiet;
+
 static inline void die(char * str, ...)
 {
 	va_list ap;

flat_dt.h +2 -0

index d5abd80..a8482b4 100644
@@ -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 */

livetree.c +5 -4

index 84f2f64..45642dc 100644
@@ -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"); */

Signature · 1 line

1.4.4.4

Jon Loeliger

2007-03-19 14:10 UTC #5 lore raw

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

So, like, the other day Jerry Van Baren mumbled:

Quoted 8 lines

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>

<sign-off as needed>
---

Notes and commentary

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

Quoted 8 lines

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

Applied and pushed out!

However, two comments:

Quoted 2 lines

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

dtc.c static void usage(void) · line 87

Quoted 2 lines

dtc.c +2 -0

+	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