All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing
@ 2015-11-24 13:21 Andreas Fenkart
  2015-11-24 13:21 ` [U-Boot] [PATCH 1/5] tools: env validate: pass values as 0-based array Andreas Fenkart
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Andreas Fenkart @ 2015-11-24 13:21 UTC (permalink / raw)
  To: u-boot

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

-- 
2.6.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH 1/5] tools: env validate: pass values as 0-based array
  2015-11-24 13:21 [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing Andreas Fenkart
@ 2015-11-24 13:21 ` Andreas Fenkart
  2015-11-24 13:21 ` [U-Boot] [PATCH 2/5] tools: env: make parse_aes_key stateless Andreas Fenkart
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Andreas Fenkart @ 2015-11-24 13:21 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH 2/5] tools: env: make parse_aes_key stateless
  2015-11-24 13:21 [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing Andreas Fenkart
  2015-11-24 13:21 ` [U-Boot] [PATCH 1/5] tools: env validate: pass values as 0-based array Andreas Fenkart
@ 2015-11-24 13:21 ` Andreas Fenkart
  2015-11-24 13:21 ` [U-Boot] [PATCH 3/5] tools: env: introduce setenv/printenv argument structs Andreas Fenkart
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Andreas Fenkart @ 2015-11-24 13:21 UTC (permalink / raw)
  To: u-boot

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;
 	}
-- 
2.6.2

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH 3/5] tools: env: introduce setenv/printenv argument structs
  2015-11-24 13:21 [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing Andreas Fenkart
  2015-11-24 13:21 ` [U-Boot] [PATCH 1/5] tools: env validate: pass values as 0-based array Andreas Fenkart
  2015-11-24 13:21 ` [U-Boot] [PATCH 2/5] tools: env: make parse_aes_key stateless Andreas Fenkart
@ 2015-11-24 13:21 ` Andreas Fenkart
  2015-11-24 13:21 ` [U-Boot] [PATCH 4/5] tools: env: parse aes key / suppress flag into argument struct Andreas Fenkart
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Andreas Fenkart @ 2015-11-24 13:21 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH 4/5] tools: env: parse aes key / suppress flag into argument struct
  2015-11-24 13:21 [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing Andreas Fenkart
                   ` (2 preceding siblings ...)
  2015-11-24 13:21 ` [U-Boot] [PATCH 3/5] tools: env: introduce setenv/printenv argument structs Andreas Fenkart
@ 2015-11-24 13:21 ` Andreas Fenkart
  2015-11-24 13:21 ` [U-Boot] [PATCH 5/5] tools: env: shift optind arguments and fix argument indices Andreas Fenkart
  2015-11-24 19:26 ` [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing Michael Heimpold
  5 siblings, 0 replies; 7+ messages in thread
From: Andreas Fenkart @ 2015-11-24 13:21 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH 5/5] tools: env: shift optind arguments and fix argument indices
  2015-11-24 13:21 [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing Andreas Fenkart
                   ` (3 preceding siblings ...)
  2015-11-24 13:21 ` [U-Boot] [PATCH 4/5] tools: env: parse aes key / suppress flag into argument struct Andreas Fenkart
@ 2015-11-24 13:21 ` Andreas Fenkart
  2015-11-24 19:26 ` [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing Michael Heimpold
  5 siblings, 0 replies; 7+ messages in thread
From: Andreas Fenkart @ 2015-11-24 13:21 UTC (permalink / raw)
  To: u-boot

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);
-- 
2.6.2

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing
  2015-11-24 13:21 [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing Andreas Fenkart
                   ` (4 preceding siblings ...)
  2015-11-24 13:21 ` [U-Boot] [PATCH 5/5] tools: env: shift optind arguments and fix argument indices Andreas Fenkart
@ 2015-11-24 19:26 ` Michael Heimpold
  5 siblings, 0 replies; 7+ messages in thread
From: Michael Heimpold @ 2015-11-24 19:26 UTC (permalink / raw)
  To: u-boot

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=9884f44cab5d1ce944b1dc087c9bb8db0efb5160

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-11-24 19:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-24 13:21 [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing Andreas Fenkart
2015-11-24 13:21 ` [U-Boot] [PATCH 1/5] tools: env validate: pass values as 0-based array Andreas Fenkart
2015-11-24 13:21 ` [U-Boot] [PATCH 2/5] tools: env: make parse_aes_key stateless Andreas Fenkart
2015-11-24 13:21 ` [U-Boot] [PATCH 3/5] tools: env: introduce setenv/printenv argument structs Andreas Fenkart
2015-11-24 13:21 ` [U-Boot] [PATCH 4/5] tools: env: parse aes key / suppress flag into argument struct Andreas Fenkart
2015-11-24 13:21 ` [U-Boot] [PATCH 5/5] tools: env: shift optind arguments and fix argument indices Andreas Fenkart
2015-11-24 19:26 ` [U-Boot] [PATCH 0/5] tools: env: simplify argument parsing Michael Heimpold

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.