Propagate Error Status to the Shell on fw_printenv Errors

PATCH New Grant Erickson 4 participants 12 messages May 7, 2008 – May 9, 2008

Share & tools

Grant Erickson

author

2008-05-07 03:16 UTC #0 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

Changed implementation such that fw_printenv returns failure status when one or more specified variables do not exist or when incorrect command syntax is used.

This aids scripting fw_printenv such that the script can key of the return status rather than relying on standard error "scraping".

Tested On:
AMCC Kilauea

Tests Run:
root at kilauea# fw_printenv
bootdelay=5
baudrate=115200
...
root at kilauea# echo $?
0

root at kilauea# fw_printenv -n hostname
kilauea
root at kilauea# echo $?
0

root at kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root at kilauea# echo $?
1

root at kilauea# fw_printenv hostname ipaddr hostname=kilauea
ipaddr=192.168.1.12
root at kilauea# echo $?
0

root at kilauea# fw_printenv foobar
## Error: "foobar" not defined
root at kilauea# echo $?
1

root at kilauea# fw_printenv hostname ipaddr foobar hostname=kilauea
ipaddr=192.168.1.12
## Error: "foobar" not defined
root at kilauea# echo $?
1

Signed-off-by Grant Erickson <[email protected]>

---
 tools/env/fw_env.c      |   19 ++++++++++++-------
 tools/env/fw_env.h      |    2 +-
 tools/env/fw_env_main.c |   30 +++++++++++++++---------------
 3 files changed, 28 insertions(+), 23 deletions(-)

tools/env/fw_env.c +12 -7

index e083a5b..d06844c 100644
@@ -209,13 +209,14 @@ char *fw_getenv (char *name)
  * Print the current definition of one, or more, or all
  * environment variables
  */
-void fw_printenv (int argc, char *argv[])
+int fw_printenv (int argc, char *argv[])
 {
 	char *env, *nxt;
 	int i, n_flag;
+	int errflag = 0;
 
 	if (env_init ())
-		return;
+		return (-1);
 
 	if (argc == 1) {		/* Print all env variables  */
 		for (env = environment.data; *env; env = nxt + 1) {

@@ -223,13 +224,13 @@ void fw_printenv (int argc, char *argv[])
 				if (nxt >= &environment.data[ENV_SIZE]) {
 					fprintf (stderr, "## Error: "
 						"environment not terminated\n");
-					return;
+					return (-1);
 				}
 			}
 
 			printf ("%s\n", env);
 		}
-		return;
+		return (0);
 	}
 
 	if (strcmp (argv[1], "-n") == 0) {

@@ -239,7 +240,7 @@ void fw_printenv (int argc, char *argv[])
 		if (argc != 2) {
 			fprintf (stderr, "## Error: "
 				"`-n' option requires exactly one argument\n");
-			return;
+			return (-1);
 		}
 	} else {
 		n_flag = 0;

@@ -255,7 +256,7 @@ void fw_printenv (int argc, char *argv[])
 				if (nxt >= &environment.data[ENV_SIZE]) {
 					fprintf (stderr, "## Error: "
 						"environment not terminated\n");
-					return;
+					return (-1);
 				}
 			}
 			val = envmatch (name, env);

@@ -268,9 +269,13 @@ void fw_printenv (int argc, char *argv[])
 				break;
 			}
 		}
-		if (!val)
+		if (!val) {
 			fprintf (stderr, "## Error: \"%s\" not defined\n", name);
+			errflag++;
+		}
 	}
+
+	return (errflag ? -1 : 0);
 }
 
 /*

tools/env/fw_env.h +1 -1

index 58607de..231dd86 100644
@@ -47,7 +47,7 @@
 	"ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; " 	\
 	"bootm"
 
-extern void  fw_printenv(int argc, char *argv[]);
+extern int fw_printenv(int argc, char *argv[]);
 extern char *fw_getenv  (char *name);
 extern int fw_setenv  (int argc, char *argv[]);

tools/env/fw_env_main.c +15 ~~-15~~

index 696e30e..f50b774 100644
@@ -25,10 +25,11 @@
  * Command line user interface to firmware (=U-Boot) environment.
  *
  * Implements:
- *	fw_printenv [ name ... ]
- *		- prints the values of the environment variables
- *		  "name", or the whole environment if no names are
- *		  specified
+ *	fw_printenv [[ -n name ] | [ name ... ]]
+ *		- prints the value of a single environment variable
+ *		  "name", the values of one or more environment
+ *		  variables "name", or the whole environment if no names
+ *		  are specified.
  *	fw_setenv name [ value ... ]
  *		- If a name without any values is given, the variable
  *		  with this name is deleted from the environment;

@@ -51,6 +52,7 @@ main(int argc, char *argv[])
 {
 	char *p;
 	char *cmdname = *argv;
+	int errflag = 0;
 
 	if ((p = strrchr (cmdname, '/')) != NULL) {
 		cmdname = p + 1;

@@ -58,21 +60,19 @@ main(int argc, char *argv[])
 
 	if (strcmp(cmdname, CMD_PRINTENV) == 0) {
 
-			fw_printenv (argc, argv);
-
-			return (EXIT_SUCCESS);
+			errflag += (fw_printenv (argc, argv) != 0);
 
 	} else if (strcmp(cmdname, CMD_SETENV) == 0) {
 
-			if (fw_setenv (argc, argv) != 0)
-				return (EXIT_FAILURE);
+			errflag += (fw_setenv (argc, argv) != 0);
 
-			return (EXIT_SUCCESS);
+	} else {
+			fprintf (stderr,
+				 "Identity crisis - may be called as `"
+				 CMD_PRINTENV
+				 "' or as `" CMD_SETENV "' but not as `%s'\n",
+				 cmdname);
 	}
 
-	fprintf (stderr,
-		"Identity crisis - may be called as `" CMD_PRINTENV
-		"' or as `" CMD_SETENV "' but not as `%s'\n",
-		cmdname);
-	return (EXIT_FAILURE);
+	return (errflag ? EXIT_FAILURE : EXIT_SUCCESS);
 }

Signature · 1 line

1.5.4.3

Markus Klotzbücher

2008-05-07 06:44 UTC #1 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

Dear Grant,

Quoted 3 lines · Grant Erickson

Changed implementation such that fw_printenv returns failure status when one or more specified variables do not exist or when incorrect command syntax is used.
This aids scripting fw_printenv such that the script can key of the return status rather than relying on standard error "scraping".

Thank you, I think this is useful.

Quoted 16 lines

Tested On:
AMCC Kilauea
Tests Run:
root at kilauea# fw_printenv
bootdelay=5
baudrate=115200
...
root at kilauea# echo $?
0
root at kilauea# fw_printenv -n hostname
kilauea
root at kilauea# echo $?
0

Quoted 4 lines

root at kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root at kilauea# echo $?
1

But I don't understand what the purpose of the "-n" option is?

Best regards

Markus Klotzbuecher

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de

Wolfgang Denk

2008-05-07 07:10 UTC #2 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

Quoted 7 lines · In message <[email protected]> you wrote:

Quoted 5 lines
root at kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root at kilauea# echo $?
1
But I don't understand what the purpose of the "-n" option is?

The purpose seems clear to me - print the value of a variable without the name. This can be useful in shell scripts, for example when you want to do something like

	$ ipaddr=`fw_printenv -n ipaddr`

However, the addition of this feature is undocumented (not mentioned in the commit message), and I don;t see why we should restrict it to a single variable - it may be useful to print several values here, too, for example like that:

	$ set `fw_printenv -n netdev ipaddr netmask`
	$ ifconfig $1 $2 netmask $3

[Well, I know that's a bad example because it's missing all error checking, but you get the idea.]

Best regards,

Wolfgang Denk

Signature · 6 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is impractical for  the  standard  to  attempt  to  constrain  the
behavior  of code that does not obey the constraints of the standard.
                                                          - Doug Gwyn

Markus Klotzbücher

2008-05-07 07:32 UTC #3 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

Quoted 7 lines · Wolfgang Denk

Quoted 6 lines · In message <[email protected]> you wrote:
Quoted 5 lines
root at kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root at kilauea# echo $?
1

Quoted 7 lines

Quoted 1 line
But I don't understand what the purpose of the "-n" option is?
The purpose seems clear to me - print the value of a variable without the name. This can be useful in shell scripts, for example when you want to do something like
	$ ipaddr=`fw_printenv -n ipaddr`

Oh right, yes. Shouldn't post to lists before drinking coffee in the morning.

Quoted 10 lines

However, the addition of this feature is undocumented (not mentioned in the commit message), and I don;t see why we should restrict it to a single variable - it may be useful to print several values here, too, for example like that:
	$ set `fw_printenv -n netdev ipaddr netmask`
	$ ifconfig $1 $2 netmask $3
[Well, I know that's a bad example because it's missing all error checking, but you get the idea.]

Yes, that would nice indeed. Grant, care to add this? And please update the documentation in any case.

Best regards

Markus Klotzb?cher

Grant Erickson

author

2008-05-07 13:53 UTC #4 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

Quoted 10 lines · Markus Klotzb?cher

Quoted 1 line · Wolfgang Denk
In message <[email protected]> you wrote:
Oh right, yes. Shouldn't post to lists before drinking coffee in the morning.
Quoted 5 lines
However, the addition of this feature is undocumented (not mentioned in the commit message), and I don;t see why we should restrict it to a single variable - it may be useful to print several values here, too, for example like that:

Quoted 8 lines

Quoted 5 lines
$ set `fw_printenv -n netdev ipaddr netmask`
$ ifconfig $1 $2 netmask $3
[Well, I know that's a bad example because it's missing all error checking, but you get the idea.]
Yes, that would nice indeed. Grant, care to add this? And please update the documentation in any case.

Markus,

Thanks for the comments. As suggested by Wolfgang, the "-n" option was pre-existing in the code before my patch.

I can investigate extending "-n" as described above; however, the caveat is that the user knows that the values of some variables may have spaces. In such a case, he/she might find the above example to not work as expected.

Anyway, hopefully Wolfgang can ACK the patch as is and I can look at extending the "-n" option as another separate effort.

Regards,

Grant

PS: To which documentation are you referring? The comments in the source or the DULG?

Markus Klotzbücher

2008-05-07 14:49 UTC #5 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

Dear Grant,

Quoted 6 lines · Grant Erickson

Thanks for the comments. As suggested by Wolfgang, the "-n" option was pre-existing in the code before my patch.
I can investigate extending "-n" as described above; however, the caveat is that the user knows that the values of some variables may have spaces. In such a case, he/she might find the above example to not work as expected.

Thats right, but then that's a problem the user needs to be aware of.

Quoted 2 lines

Anyway, hopefully Wolfgang can ACK the patch as is and I can look at extending the "-n" option as another separate effort.

Ok, fine!

Quoted 2 lines

PS: To which documentation are you referring? The comments in the source or the DULG?

The README in the tools/env/ directory. As you're fixing a bug (opposed to adding a feature) there's no obligation really to do this, but it would be great anyway :-)

And I can take care of updating the DULG.

Thanks!

Markus Klotzbuecher

Wolfgang Denk

2008-05-07 15:50 UTC #6 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

In reply to Grant Erickson

Quoted 3 lines · In message <C4470263.F0B2%[email protected]> you wrote:

Anyway, hopefully Wolfgang can ACK the patch as is and I can look at extending the "-n" option as another separate effort.

No, I will not ACK it, as it adds features which are not even mentioned in the commit message.

Quoted 2 lines

PS: To which documentation are you referring? The comments in the source or the DULG?

The commit message in the first place. Ideally also in the DULG.

Best regards,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In the bathtub of history the truth is harder to hold than the  soap,
and much more difficult to find ...     - Terry Pratchett, _Sourcery_

Grant Erickson

author

2008-05-07 13:56 UTC #7 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

In reply to Wolfgang Denk

Quoted 20 lines · Wolfgang Denk

Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 5 lines
root at kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root at kilauea# echo $?
1
But I don't understand what the purpose of the "-n" option is?
The purpose seems clear to me - print the value of a variable without the name. This can be useful in shell scripts, for example when you want to do something like
$ ipaddr=`fw_printenv -n ipaddr`
However, the addition of this feature is undocumented (not mentioned in the commit message), and I don;t see why we should restrict it to a single variable - it may be useful to print several values here, too, for example like that:

Quoted 5 lines

$ set `fw_printenv -n netdev ipaddr netmask`
$ ifconfig $1 $2 netmask $3
[Well, I know that's a bad example because it's missing all error checking, but you get the idea.]

The "-n" option is not a new feature. It already existed in the code prior to my changes.

Per my comments to Markus in a follow-up to his post, I am happy to evaluate extending the "-n" functionality, but would like to do so as a separate effort to the submitted patch which, I believe, stands on its own.

Regards,

Grant

Wolfgang Denk

2008-05-07 15:55 UTC #8 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

Quoted 3 lines · In message <C4470304.F0B4%[email protected]> you wrote:

The "-n" option is not a new feature. It already existed in the code prior to my changes.

Argh! You are right. This option has been there right from the first version. And guess who wrote that code.

I apologize for all the false alarms.

Now where is that brown paper bag?

Best regards,

Wolfgang Denk

Signature · 6 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Data is a lot like humans: It is  born.  Matures.  Gets  married  to
other  data, divorced. Gets old. One thing that it doesn't do is die.
It has to be killed."                                 - Arthur Miller

Joakim Tjernlund

2008-05-07 07:29 UTC #9 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

In reply to Grant Erickson

Quoted 25 lines · Grant Erickson

Changed implementation such that fw_printenv returns failure status when one or more specified variables do not exist or when incorrect command syntax is used.
This aids scripting fw_printenv such that the script can key of the return status rather than relying on standard error "scraping".
Tested On:
AMCC Kilauea
Tests Run:
root at kilauea# fw_printenv
bootdelay=5
baudrate=115200
...
root at kilauea# echo $?
0
root at kilauea# fw_printenv -n hostname
kilauea
root at kilauea# echo $?
0
root at kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root at kilauea# echo $?
1

Quoted 8 lines

root at kilauea# fw_printenv hostname ipaddr hostname=kilauea
ipaddr=192.168.1.12
root at kilauea# echo $?
0
root at kilauea# fw_printenv foobar
## Error: "foobar" not defined

This error message should not be there. It is enough to return exit status 1. Then one does not have to redirect stderr in scripts

Quoted 7 lines

root at kilauea# echo $?
1
root at kilauea# fw_printenv hostname ipaddr foobar hostname=kilauea
ipaddr=192.168.1.12
## Error: "foobar" not defined

Same here.

Quoted 3 lines

	root at kilauea# echo $?
	1

Could you also remove the messages printed during fw_setenv(show below)? root at jtd:~# fw_setenv kalle sven
Unlocking flash...
Done
Erasing old environment...
Done
Writing environment to /dev/mtd2...
Done
Locking ...
Done

Perhaps hide them behind a -v(verbose) option?

Grant Erickson

author

2008-05-07 14:01 UTC #10 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

Quoted 29 lines · Joakim Tjernlund

Quoted 2 lines · Grant Erickson
root at kilauea# fw_printenv foobar
## Error: "foobar" not defined
This error message should not be there. It is enough to return exit status 1. Then one does not have to redirect stderr in scripts
Quoted 4 lines
root at kilauea# fw_printenv hostname ipaddr foobar hostname=kilauea
ipaddr=192.168.1.12
## Error: "foobar" not defined
Same here.
Quoted 3 lines
root at kilauea# echo $?
1
Could you also remove the messages printed during fw_setenv(show below)? root at jtd:~# fw_setenv kalle sven
Unlocking flash...
Done
Erasing old environment...
Done
Writing environment to /dev/mtd2...
Done
Locking ...
Done

Quoted 1 line

Perhaps hide them behind a -v(verbose) option?

Joakim,

Agreed that there should be a way to suppress or, alternatively, enable these verbose status messages. However, these messages were all in the code already.

So, to the extent that the maintainer (Wolfgang?) is amenable to adding another option ("-v" or "-q"), I can evaluate working on this change as a separate patch.

Another change I am also looking into is eliminating the write to flash even when a variable does not exist:

root at kilauea# fw_printenv foobar
## Error: "foobar" not defined
root at kilauea# echo $?
1

root at kilauea# fw_setenv foobar
Unlocking flash...
Done
Erasing old environment...
Done
Writing environment to /dev/mtd4...
Done
Locking ...
Done
root at kilauea# echo $?
0

Regards,

Grant

Wolfgang Denk

2008-05-09 21:41 UTC #11 lore raw

PATCH 1/1 Propagate Error Status to the Shell on fw_printenv Errors

In reply to Grant Erickson

Quoted 3 lines · In message <[email protected]> you…

Changed implementation such that fw_printenv returns failure status when one or more specified variables do not exist or when incorrect command syntax is used.
This aids scripting fw_printenv such that the script can key of the return status rather than relying on standard error "scraping".

...

Quoted 6 lines

Signed-off-by Grant Erickson <[email protected]>

---
 tools/env/fw_env.c      |   19 ++++++++++++-------
 tools/env/fw_env.h      |    2 +-
 tools/env/fw_env_main.c |   30 +++++++++++++++---------------
 3 files changed, 28 insertions(+), 23 deletions(-)

Applied (with minor stylistic modifications). Thanks.

Best regards,

Wolfgang Denk

Signature · 4 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If you hear an onion ring, answer it.