[U-Boot-Users] [PATCH] 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@kilauea# fw_printenv bootdelay=5 baudrate=115200 ... root@kilauea# echo $? 0 root@kilauea# fw_printenv -n hostname kilauea root@kilauea# echo $? 0 root@kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root@kilauea# echo $? 1 root@kilauea# fw_printenv hostname ipaddr hostname=kilauea ipaddr=192.168.1.12 root@kilauea# echo $? 0 root@kilauea# fw_printenv foobar ## Error: "foobar" not defined root@kilauea# echo $? 1
root@kilauea# fw_printenv hostname ipaddr foobar hostname=kilauea ipaddr=192.168.1.12 ## Error: "foobar" not defined root@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); }
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@kilauea# fw_printenv bootdelay=5 baudrate=115200 ... root@kilauea# echo $? 0
root@kilauea# fw_printenv -n hostname kilauea root@kilauea# echo $? 0
root@kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root@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@denx.de
In message 87myn2ehqm.fsf@denx.de you wrote:
root@kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root@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
Wolfgang Denk wd@denx.de writes:
In message 87myn2ehqm.fsf@denx.de you wrote:
root@kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root@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@denx.de
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?
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@denx.de
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
On 5/7/08 12:10 AM, Wolfgang Denk wrote:
In message 87myn2ehqm.fsf@denx.de you wrote:
root@kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root@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
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
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@kilauea# fw_printenv bootdelay=5 baudrate=115200 ... root@kilauea# echo $? 0
root@kilauea# fw_printenv -n hostname kilauea root@kilauea# echo $? 0
root@kilauea# fw_printenv -n hostname ipaddr ## Error: `-n' option requires exactly one argument root@kilauea# echo $? 1
root@kilauea# fw_printenv hostname ipaddr hostname=kilauea ipaddr=192.168.1.12 root@kilauea# echo $? 0
root@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@kilauea# echo $? 1
root@kilauea# fw_printenv hostname ipaddr foobar hostname=kilauea ipaddr=192.168.1.12 ## Error: "foobar" not defined
Same here.
root@kilauea# echo $? 1
Could you also remove the messages printed during fw_setenv(show below)? root@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?
On 5/7/08 12:29 AM, Joakim Tjernlund wrote:
On Tue, 2008-05-06 at 20:16 -0700, Grant Erickson wrote:
root@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@kilauea# fw_printenv hostname ipaddr foobar hostname=kilauea ipaddr=192.168.1.12 ## Error: "foobar" not defined
Same here.
root@kilauea# echo $? 1
Could you also remove the messages printed during fw_setenv(show below)? root@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@kilauea# fw_printenv foobar ## Error: "foobar" not defined root@kilauea# echo $? 1
root@kilauea# fw_setenv foobar Unlocking flash... Done Erasing old environment... Done Writing environment to /dev/mtd4... Done Locking ... Done root@kilauea# echo $? 0
Regards,
Grant
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
participants (4)
-
Grant Erickson -
Joakim Tjernlund -
Markus Klotzbücher -
Wolfgang Denk