[U-Boot] [PATCH 0/5] tools: env: simplify argument parsing
I want to add -c config_file parameter to fw_setenv/fw_printenv, so I can switch between old/new u-boot environment after u-boot upgrade. In it's current state paramter parsing is quite hard to understand since it happens in two places. One is using getopt at the beginning of main, the second is using adhoc parsing where the order of arguments is important. This patch will parse arguments only in one place using getopt and store the parsed flags in a global struct.
Andreas Fenkart (5): tools: env validate: pass values as 0-based array tools: env: make parse_aes_key stateless tools: env: introduce setenv/printenv argument structs tools: env: parse aes key / suppress flag into argument struct tools: env: shift optind arguments and fix argument indices
common/env_flags.c | 14 +++--- include/env_flags.h | 2 +- tools/env/fw_env.c | 94 ++++++++++-------------------------- tools/env/fw_env.h | 21 ++++++++ tools/env/fw_env_main.c | 124 +++++++++++++++++++++++++++++++++--------------- 5 files changed, 140 insertions(+), 115 deletions(-)
passing argv/argc can produce off-by-one errors
Signed-off-by: Andreas Fenkart andreas.fenkart@dev.digitalstrom.org --- common/env_flags.c | 14 +++++++------- include/env_flags.h | 2 +- tools/env/fw_env.c | 11 +++++++---- 3 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/common/env_flags.c b/common/env_flags.c index 985f92e..fb5ac40 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -359,21 +359,21 @@ int env_flags_validate_varaccess(const char *name, int check_mask) /* * Validate the parameters to "env set" directly */ -int env_flags_validate_env_set_params(int argc, char * const argv[]) +int env_flags_validate_env_set_params(char *name, char * const val[], int count) { - if ((argc >= 3) && argv[2] != NULL) { - enum env_flags_vartype type = env_flags_get_type(argv[1]); + if ((count >= 1) && val[0] != NULL) { + enum env_flags_vartype type = env_flags_get_type(name);
/* * we don't currently check types that need more than * one argument */ - if (type != env_flags_vartype_string && argc > 3) { - printf("## Error: too many parameters for setting " - ""%s"\n", argv[1]); + if (type != env_flags_vartype_string && count > 1) { + printf("## Error: too many parameters for setting "%s"\n", + name); return -1; } - return env_flags_validate_type(argv[1], argv[2]); + return env_flags_validate_type(name, val[0]); } /* ok */ return 0; diff --git a/include/env_flags.h b/include/env_flags.h index 3ef6311..b153a71 100644 --- a/include/env_flags.h +++ b/include/env_flags.h @@ -121,7 +121,7 @@ int env_flags_validate_varaccess(const char *name, int check_mask); /* * Validate the parameters passed to "env set" for type compliance */ -int env_flags_validate_env_set_params(int argc, char * const argv[]); +int env_flags_validate_env_set_params(char *name, char *const val[], int count);
#else /* !USE_HOSTCC */
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 1173eea..bcf3756 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -481,8 +481,9 @@ int fw_setenv(int argc, char *argv[]) { int i, rc; size_t len; - char *name; + char *name, **valv; char *value = NULL; + int valc;
if (argc < 2) { errno = EINVAL; @@ -513,13 +514,15 @@ int fw_setenv(int argc, char *argv[]) }
name = argv[1]; + valv = argv + 2; + valc = argc - 2;
- if (env_flags_validate_env_set_params(argc, argv) < 0) + if (env_flags_validate_env_set_params(name, valv, valc) < 0) return 1;
len = 0; - for (i = 2; i < argc; ++i) { - char *val = argv[i]; + for (i = 0; i < valc; ++i) { + char *val = valv[i]; size_t val_len = strlen(val);
if (value)
Signed-off-by: Andreas Fenkart andreas.fenkart@dev.digitalstrom.org --- tools/env/fw_env.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index bcf3756..f1dea8b 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -204,7 +204,7 @@ char *fw_getdefenv(char *name) return NULL; }
-static int parse_aes_key(char *key) +static int parse_aes_key(char *key, uint8_t *bin_key) { char tmp[5] = { '0', 'x', 0, 0, 0 }; unsigned long ul; @@ -226,11 +226,9 @@ static int parse_aes_key(char *key) "## Error: '-a' option requires valid AES key\n"); return -1; } - aes_key[i] = ul & 0xff; + bin_key[i] = ul & 0xff; key += 2; } - aes_flag = 1; - return 0; }
@@ -250,9 +248,10 @@ int fw_printenv (int argc, char *argv[]) "## Error: '-a' option requires AES key\n"); return -1; } - rc = parse_aes_key(argv[2]); + rc = parse_aes_key(argv[2], aes_key); if (rc) return rc; + aes_flag = 1; argv += 2; argc -= 2; } @@ -496,9 +495,10 @@ int fw_setenv(int argc, char *argv[]) "## Error: '-a' option requires AES key\n"); return -1; } - rc = parse_aes_key(argv[2]); + rc = parse_aes_key(argv[2], aes_key); if (rc) return rc; + aes_flag = 1; argv += 2; argc -= 2; }
goal is to use getopt for all argument parsing instead of adhoc parsing in fw_getenv/fw_setenv functions
Signed-off-by: Andreas Fenkart andreas.fenkart@dev.digitalstrom.org --- tools/env/fw_env.h | 9 ++++ tools/env/fw_env_main.c | 111 +++++++++++++++++++++++++++++++----------------- 2 files changed, 81 insertions(+), 39 deletions(-)
diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h index aff471b..4ac7de4 100644 --- a/tools/env/fw_env.h +++ b/tools/env/fw_env.h @@ -52,6 +52,15 @@ "bootm" #endif
+struct printenv_args { +}; +extern struct printenv_args printenv_args; + +struct setenv_args { + char *script_file; +}; +extern struct setenv_args setenv_args; + 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 ce50d58..f45c94a 100644 --- a/tools/env/fw_env_main.c +++ b/tools/env/fw_env_main.c @@ -45,6 +45,9 @@ static struct option long_options[] = { {NULL, 0, NULL, 0} };
+struct printenv_args printenv_args; +struct setenv_args setenv_args; + void usage(void) {
@@ -72,33 +75,11 @@ void usage(void) ); }
-int main(int argc, char *argv[]) +int parse_printenv_args(int argc, char *argv[]) { - char *p; - char *cmdname = *argv; - char *script_file = NULL; int c; - const char *lockname = "/var/lock/" CMD_PRINTENV ".lock"; - int lockfd = -1; - int retval = EXIT_SUCCESS; - - lockfd = open(lockname, O_WRONLY | O_CREAT | O_TRUNC, 0666); - if (-1 == lockfd) { - fprintf(stderr, "Error opening lock file %s\n", lockname); - return EXIT_FAILURE; - } - - if (-1 == flock(lockfd, LOCK_EX)) { - fprintf(stderr, "Error locking file %s\n", lockname); - close(lockfd); - return EXIT_FAILURE; - }
- if ((p = strrchr (cmdname, '/')) != NULL) { - cmdname = p + 1; - } - - while ((c = getopt_long (argc, argv, "a:ns:h", + while ((c = getopt_long (argc, argv, "a:nh", long_options, NULL)) != EOF) { switch (c) { case 'a': @@ -107,40 +88,92 @@ int main(int argc, char *argv[]) case 'n': /* handled in fw_printenv */ break; + case 'h': + usage(); + exit(EXIT_SUCCESS); + break; + default: /* '?' */ + usage(); + exit(EXIT_FAILURE); + break; + } + } + return 0; +} + +int parse_setenv_args(int argc, char *argv[]) +{ + int c; + while ((c = getopt_long (argc, argv, "a:s:h", + long_options, NULL)) != EOF) { + switch (c) { + case 'a': + /* AES key, handled later */ + break; case 's': - script_file = optarg; + setenv_args.script_file = optarg; break; case 'h': usage(); - goto exit; + exit(EXIT_SUCCESS); + break; default: /* '?' */ - fprintf(stderr, "Try `%s --help' for more information." - "\n", cmdname); - retval = EXIT_FAILURE; - goto exit; + usage(); + exit(EXIT_FAILURE); + break; } } + return 0; +} + +int main(int argc, char *argv[]) +{ + char *cmdname = *argv; + const char *lockname = "/var/lock/" CMD_PRINTENV ".lock"; + int lockfd = -1; + int retval = EXIT_SUCCESS; + + if (strrchr(cmdname, '/') != NULL) + cmdname = strrchr(cmdname, '/') + 1; + + if (strcmp(cmdname, CMD_PRINTENV) == 0) { + if (parse_printenv_args(argc, argv)) + exit(EXIT_FAILURE); + } else if (strcmp(cmdname, CMD_SETENV) == 0) { + if (parse_setenv_args(argc, argv)) + exit(EXIT_FAILURE); + } else { + fprintf(stderr, + "Identity crisis - may be called as `%s' or as `%s' but not as `%s'\n", + CMD_PRINTENV, CMD_SETENV, cmdname); + exit(EXIT_FAILURE); + } + + lockfd = open(lockname, O_WRONLY | O_CREAT | O_TRUNC, 0666); + if (-1 == lockfd) { + fprintf(stderr, "Error opening lock file %s\n", lockname); + return EXIT_FAILURE; + } + + if (-1 == flock(lockfd, LOCK_EX)) { + fprintf(stderr, "Error locking file %s\n", lockname); + close(lockfd); + return EXIT_FAILURE; + }
if (strcmp(cmdname, CMD_PRINTENV) == 0) { if (fw_printenv(argc, argv) != 0) retval = EXIT_FAILURE; } else if (strcmp(cmdname, CMD_SETENV) == 0) { - if (!script_file) { + if (!setenv_args.script_file) { if (fw_setenv(argc, argv) != 0) retval = EXIT_FAILURE; } else { - if (fw_parse_script(script_file) != 0) + if (fw_parse_script(setenv_args.script_file) != 0) retval = EXIT_FAILURE; } - } else { - fprintf(stderr, - "Identity crisis - may be called as `" CMD_PRINTENV - "' or as `" CMD_SETENV "' but not as `%s'\n", - cmdname); - retval = EXIT_FAILURE; }
-exit: flock(lockfd, LOCK_UN); close(lockfd); return retval;
disabled original parsing, but not yet removed since the argument indexing needs to be fixed
Signed-off-by: Andreas Fenkart andreas.fenkart@dev.digitalstrom.org --- tools/env/fw_env.c | 45 +++++++++------------------------------------ tools/env/fw_env.h | 12 ++++++++++++ tools/env/fw_env_main.c | 15 ++++++++++++--- 3 files changed, 33 insertions(+), 39 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index f1dea8b..32bb3aa 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -31,8 +31,6 @@
#include "fw_env.h"
-#include <aes.h> - #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
#define WHITESPACE(c) ((c == '\t') || (c == ' ')) @@ -102,9 +100,6 @@ static struct environment environment = { .flag_scheme = FLAG_NONE, };
-/* Is AES encryption used? */ -static int aes_flag; -static uint8_t aes_key[AES_KEY_LENGTH] = { 0 }; static int env_aes_cbc_crypt(char *data, const int enc);
static int HaveRedundEnv = 0; @@ -130,7 +125,7 @@ static inline ulong getenvsize (void) if (HaveRedundEnv) rc -= sizeof (char);
- if (aes_flag) + if (common_args.aes_flag) rc &= ~(AES_KEY_LENGTH - 1);
return rc; @@ -204,7 +199,7 @@ char *fw_getdefenv(char *name) return NULL; }
-static int parse_aes_key(char *key, uint8_t *bin_key) +int parse_aes_key(char *key, uint8_t *bin_key) { char tmp[5] = { '0', 'x', 0, 0, 0 }; unsigned long ul; @@ -239,19 +234,9 @@ static int parse_aes_key(char *key, uint8_t *bin_key) int fw_printenv (int argc, char *argv[]) { char *env, *nxt; - int i, n_flag; - int rc = 0; + int i, rc = 0;
if (argc >= 2 && strcmp(argv[1], "-a") == 0) { - if (argc < 3) { - fprintf(stderr, - "## Error: '-a' option requires AES key\n"); - return -1; - } - rc = parse_aes_key(argv[2], aes_key); - if (rc) - return rc; - aes_flag = 1; argv += 2; argc -= 2; } @@ -275,7 +260,6 @@ int fw_printenv (int argc, char *argv[]) }
if (strcmp (argv[1], "-n") == 0) { - n_flag = 1; ++argv; --argc; if (argc != 2) { @@ -283,8 +267,6 @@ int fw_printenv (int argc, char *argv[]) "`-n' option requires exactly one argument\n"); return -1; } - } else { - n_flag = 0; }
for (i = 1; i < argc; ++i) { /* print single env variables */ @@ -302,7 +284,7 @@ int fw_printenv (int argc, char *argv[]) } val = envmatch (name, env); if (val) { - if (!n_flag) { + if (!printenv_args.name_suppress) { fputs (name, stdout); putc ('=', stdout); } @@ -322,7 +304,7 @@ int fw_printenv (int argc, char *argv[]) int fw_env_close(void) { int ret; - if (aes_flag) { + if (common_args.aes_flag) { ret = env_aes_cbc_crypt(environment.data, 1); if (ret) { fprintf(stderr, @@ -478,7 +460,7 @@ int fw_env_write(char *name, char *value) */ int fw_setenv(int argc, char *argv[]) { - int i, rc; + int i; size_t len; char *name, **valv; char *value = NULL; @@ -490,15 +472,6 @@ int fw_setenv(int argc, char *argv[]) }
if (strcmp(argv[1], "-a") == 0) { - if (argc < 3) { - fprintf(stderr, - "## Error: '-a' option requires AES key\n"); - return -1; - } - rc = parse_aes_key(argv[2], aes_key); - if (rc) - return rc; - aes_flag = 1; argv += 2; argc -= 2; } @@ -996,7 +969,7 @@ static int env_aes_cbc_crypt(char *payload, const int enc) uint32_t aes_blocks;
/* First we expand the key. */ - aes_expand_key(aes_key, key_exp); + aes_expand_key(common_args.aes_key, key_exp);
/* Calculate the number of AES blocks to encrypt. */ aes_blocks = DIV_ROUND_UP(len, AES_KEY_LENGTH); @@ -1224,7 +1197,7 @@ int fw_env_open(void)
crc0 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
- if (aes_flag) { + if (common_args.aes_flag) { ret = env_aes_cbc_crypt(environment.data, 0); if (ret) return ret; @@ -1281,7 +1254,7 @@ int fw_env_open(void)
crc1 = crc32 (0, (uint8_t *) redundant->data, ENV_SIZE);
- if (aes_flag) { + if (common_args.aes_flag) { ret = env_aes_cbc_crypt(redundant->data, 0); if (ret) return ret; diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h index 4ac7de4..abb0f3b 100644 --- a/tools/env/fw_env.h +++ b/tools/env/fw_env.h @@ -5,6 +5,9 @@ * SPDX-License-Identifier: GPL-2.0+ */
+#include <aes.h> +#include <stdint.h> + /* Pull in the current config to define the default environment */ #ifndef __ASSEMBLY__ #define __ASSEMBLY__ /* get only #defines from config.h */ @@ -52,7 +55,14 @@ "bootm" #endif
+struct common_args { + uint8_t aes_key[AES_KEY_LENGTH]; + int aes_flag; /* Is AES encryption used? */ +}; +extern struct common_args common_args; + struct printenv_args { + int name_suppress; }; extern struct printenv_args printenv_args;
@@ -61,6 +71,8 @@ struct setenv_args { }; extern struct setenv_args setenv_args;
+int parse_aes_key(char *key, uint8_t *bin_key); + 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 f45c94a..c828fd0 100644 --- a/tools/env/fw_env_main.c +++ b/tools/env/fw_env_main.c @@ -45,6 +45,7 @@ static struct option long_options[] = { {NULL, 0, NULL, 0} };
+struct common_args common_args; struct printenv_args printenv_args; struct setenv_args setenv_args;
@@ -83,10 +84,14 @@ int parse_printenv_args(int argc, char *argv[]) long_options, NULL)) != EOF) { switch (c) { case 'a': - /* AES key, handled later */ + if (parse_aes_key(optarg, common_args.aes_key)) { + fprintf(stderr, "AES key parse error\n"); + return EXIT_FAILURE; + } + common_args.aes_flag = 1; break; case 'n': - /* handled in fw_printenv */ + printenv_args.name_suppress = 1; break; case 'h': usage(); @@ -108,7 +113,11 @@ int parse_setenv_args(int argc, char *argv[]) long_options, NULL)) != EOF) { switch (c) { case 'a': - /* AES key, handled later */ + if (parse_aes_key(optarg, common_args.aes_key)) { + fprintf(stderr, "AES key parse error\n"); + return EXIT_FAILURE; + } + common_args.aes_flag = 1; break; case 's': setenv_args.script_file = optarg;
Signed-off-by: Andreas Fenkart andreas.fenkart@dev.digitalstrom.org --- tools/env/fw_env.c | 40 +++++++++++----------------------------- tools/env/fw_env_main.c | 4 ++++ 2 files changed, 15 insertions(+), 29 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 32bb3aa..a89bde5 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -236,15 +236,10 @@ int fw_printenv (int argc, char *argv[]) char *env, *nxt; int i, rc = 0;
- if (argc >= 2 && strcmp(argv[1], "-a") == 0) { - argv += 2; - argc -= 2; - } - if (fw_env_open()) return -1;
- if (argc == 1) { /* Print all env variables */ + if (argc == 0) { /* Print all env variables */ for (env = environment.data; *env; env = nxt + 1) { for (nxt = env; *nxt; ++nxt) { if (nxt >= &environment.data[ENV_SIZE]) { @@ -259,17 +254,13 @@ int fw_printenv (int argc, char *argv[]) return 0; }
- if (strcmp (argv[1], "-n") == 0) { - ++argv; - --argc; - if (argc != 2) { - fprintf (stderr, "## Error: " - "`-n' option requires exactly one argument\n"); - return -1; - } + if (printenv_args.name_suppress && argc != 1) { + fprintf(stderr, + "## Error: `-n' option requires exactly one argument\n"); + return -1; }
- for (i = 1; i < argc; ++i) { /* print single env variables */ + for (i = 0; i < argc; ++i) { /* print single env variables */ char *name = argv[i]; char *val = NULL;
@@ -466,17 +457,8 @@ int fw_setenv(int argc, char *argv[]) char *value = NULL; int valc;
- if (argc < 2) { - errno = EINVAL; - return -1; - } - - if (strcmp(argv[1], "-a") == 0) { - argv += 2; - argc -= 2; - } - - if (argc < 2) { + if (argc < 1) { + fprintf(stderr, "## Error: variable name missing\n"); errno = EINVAL; return -1; } @@ -486,9 +468,9 @@ int fw_setenv(int argc, char *argv[]) return -1; }
- name = argv[1]; - valv = argv + 2; - valc = argc - 2; + name = argv[0]; + valv = argv + 1; + valc = argc - 1;
if (env_flags_validate_env_set_params(name, valv, valc) < 0) return 1; diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c index c828fd0..c20325f 100644 --- a/tools/env/fw_env_main.c +++ b/tools/env/fw_env_main.c @@ -158,6 +158,10 @@ int main(int argc, char *argv[]) exit(EXIT_FAILURE); }
+ /* shift parsed flags, jump to non-option arguments */ + argc -= optind; + argv += optind; + lockfd = open(lockname, O_WRONLY | O_CREAT | O_TRUNC, 0666); if (-1 == lockfd) { fprintf(stderr, "Error opening lock file %s\n", lockname);
Hi Andreas,
Am Tuesday 24 November 2015, 14:21:10 schrieb Andreas Fenkart:
I want to add -c config_file parameter to fw_setenv/fw_printenv, so I can switch between old/new u-boot environment after u-boot upgrade.
for a similar use-case a patch was accepted/merged some days ago: http://git.denx.de/?p=u-boot.git;a=commit;h=9884f44cab5d1ce944b1dc087c9bb8db...
This is why you series does not apply to current master branch.
In it's current state paramter parsing is quite hard to understand since it happens in two places. One is using getopt at the beginning of main, the second is using adhoc parsing where the order of arguments is important. This patch will parse arguments only in one place using getopt and store the parsed flags in a global struct.
It would be desirable, if you could rebase and respin your series, since it is much appreciated.
Best regards, Michael
Andreas Fenkart (5): tools: env validate: pass values as 0-based array tools: env: make parse_aes_key stateless tools: env: introduce setenv/printenv argument structs tools: env: parse aes key / suppress flag into argument struct tools: env: shift optind arguments and fix argument indices
common/env_flags.c | 14 +++--- include/env_flags.h | 2 +- tools/env/fw_env.c | 94 ++++++++++-------------------------- tools/env/fw_env.h | 21 ++++++++ tools/env/fw_env_main.c | 124 +++++++++++++++++++++++++++++++++--------------- 5 files changed, 140 insertions(+), 115 deletions(-)
participants (2)
-
Andreas Fenkart -
Michael Heimpold