[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[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
	
	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 <gerickson@nuovations.com>
---
 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(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index e083a5b..d06844c 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -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);
 }
 
 /*
diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h
index 58607de..231dd86 100644
--- a/tools/env/fw_env.h
+++ b/tools/env/fw_env.h
@@ -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[]);
 
diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c
index 696e30e..f50b774 100644
--- a/tools/env/fw_env_main.c
+++ b/tools/env/fw_env_main.c
@@ -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);
 }
-- 
1.5.4.3

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Markus Klotzbücher]

Dear Grant,

Grant Erickson <gerickson@nuovations.com> writes:

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

>   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

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

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Wolfgang Denk]

In message <87myn2ehqm.fsf@denx.de> you wrote:
> 
> > 	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

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

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Markus Klotzbücher]

Wolfgang Denk <wd@denx.de> writes:

> In message <87myn2ehqm.fsf@denx.de> you wrote:
>> 
>> > 	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`

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

> 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

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

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Grant Erickson]

On 5/7/08 12:32 AM, Markus Klotzb?cher wrote:
> Wolfgang Denk <wd@denx.de> writes:
>> In message <87myn2ehqm.fsf@denx.de> you wrote:
> Oh right, yes. Shouldn't post to lists before drinking coffee in the
> morning.
> 
>> 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.

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?

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Markus Klotzbücher]

Dear Grant,

Grant Erickson <gerickson@nuovations.com> writes:

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

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

Ok, fine!

> 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

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

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Wolfgang Denk]

In message <C4470263.F0B2%gerickson@nuovations.com> 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.

> 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

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

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Grant Erickson]

On 5/7/08 12:10 AM, Wolfgang Denk wrote:
> In message <87myn2ehqm.fsf@denx.de> you wrote:
>> 
>>> 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.]

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

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Wolfgang Denk]

In message <C4470304.F0B4%gerickson@nuovations.com> 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

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

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Joakim Tjernlund]

On Tue, 2008-05-06 at 20:16 -0700, Grant Erickson wrote:
> 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

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

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

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

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Grant Erickson]

On 5/7/08 12:29 AM, Joakim Tjernlund wrote:
> On Tue, 2008-05-06 at 20:16 -0700, Grant Erickson wrote:
>> 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
> 
>> root at kilauea# fw_printenv hostname ipaddr foobar
>> hostname=kilauea
>> ipaddr=192.168.1.12
>> ## Error: "foobar" not defined
> 
> Same here.
> 
>> 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?

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

[U-Boot-Users] [PATCH] Propagate Error Status to the Shell on fw_printenv Errors

[Wolfgang Denk]

In message <1210130175-1896-1-git-send-email-gerickson@nuovations.com> you wrote:
> 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".
...
> Signed-off-by: Grant Erickson <gerickson@nuovations.com>
> ---
>  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

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