[U-Boot] [PATCH 0/5] tools/env: pass arguments as parameters not global structure

u-boot tools can be built as a library (ibubootenv.a). Passing arguments to the library using global structures is not a good interface.
Andreas Fenkart (5): tools/env: make env_aes_cbc_crypt re-entrant tools/env: remove 'extern' from function prototype in fw_env.h tools/env: fw_printenv pass value_only as argument tools/env: compute size of usable area only once tools/env: no global variable sharing between application and library
tools/env/fw_env.c | 95 ++++++++++++++++++++++++++----------------------- tools/env/fw_env.h | 31 ++++++---------- tools/env/fw_env_main.c | 28 +++++++++------ 3 files changed, 78 insertions(+), 76 deletions(-)

Signed-off-by: Andreas Fenkart andreas.fenkart@digitalstrom.com --- tools/env/fw_env.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 1420ac5..c362a41 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -106,7 +106,8 @@ static struct environment environment = { .flag_scheme = FLAG_NONE, };
-static int env_aes_cbc_crypt(char *data, const int enc); +static int env_aes_cbc_crypt(char *data, const int enc, + uint8_t key[AES_KEY_LENGTH]);
static int HaveRedundEnv = 0;
@@ -304,7 +305,8 @@ int fw_env_close(void) { int ret; if (common_args.aes_flag) { - ret = env_aes_cbc_crypt(environment.data, 1); + ret = env_aes_cbc_crypt(environment.data, 1, + common_args.aes_key); if (ret) { fprintf(stderr, "Error: can't encrypt env for flash\n"); @@ -949,7 +951,8 @@ static int flash_flag_obsolete (int dev, int fd, off_t offset) }
/* Encrypt or decrypt the environment before writing or reading it. */ -static int env_aes_cbc_crypt(char *payload, const int enc) +static int env_aes_cbc_crypt(char *payload, const int enc, + uint8_t key[AES_KEY_LENGTH]) { uint8_t *data = (uint8_t *)payload; const int len = getenvsize(); @@ -957,7 +960,7 @@ static int env_aes_cbc_crypt(char *payload, const int enc) uint32_t aes_blocks;
/* First we expand the key. */ - aes_expand_key(common_args.aes_key, key_exp); + aes_expand_key(key, key_exp);
/* Calculate the number of AES blocks to encrypt. */ aes_blocks = DIV_ROUND_UP(len, AES_KEY_LENGTH); @@ -1186,7 +1189,8 @@ int fw_env_open(void) crc0 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
if (common_args.aes_flag) { - ret = env_aes_cbc_crypt(environment.data, 0); + ret = env_aes_cbc_crypt(environment.data, 0, + common_args.aes_key); if (ret) return ret; } @@ -1243,7 +1247,8 @@ int fw_env_open(void) crc1 = crc32 (0, (uint8_t *) redundant->data, ENV_SIZE);
if (common_args.aes_flag) { - ret = env_aes_cbc_crypt(redundant->data, 0); + ret = env_aes_cbc_crypt(redundant->data, 0, + common_args.aes_key); if (ret) return ret; }

Hi Andreas,
thanks for fixing this:
On 05/04/2016 23:13, Andreas Fenkart wrote:
Signed-off-by: Andreas Fenkart andreas.fenkart@digitalstrom.com
tools/env/fw_env.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 1420ac5..c362a41 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -106,7 +106,8 @@ static struct environment environment = { .flag_scheme = FLAG_NONE, };
-static int env_aes_cbc_crypt(char *data, const int enc); +static int env_aes_cbc_crypt(char *data, const int enc,
uint8_t key[AES_KEY_LENGTH]);
Formally, is it not simply as pointer ?
static int env_aes_cbc_crypt(char *data, const int enc, uint8_t *key);
static int HaveRedundEnv = 0;
@@ -304,7 +305,8 @@ int fw_env_close(void) { int ret; if (common_args.aes_flag) {
ret = env_aes_cbc_crypt(environment.data, 1);
ret = env_aes_cbc_crypt(environment.data, 1,
if (ret) { fprintf(stderr, "Error: can't encrypt env for flash\n");common_args.aes_key);
@@ -949,7 +951,8 @@ static int flash_flag_obsolete (int dev, int fd, off_t offset) }
/* Encrypt or decrypt the environment before writing or reading it. */ -static int env_aes_cbc_crypt(char *payload, const int enc) +static int env_aes_cbc_crypt(char *payload, const int enc,
uint8_t key[AES_KEY_LENGTH])
Ditto.
{ uint8_t *data = (uint8_t *)payload; const int len = getenvsize(); @@ -957,7 +960,7 @@ static int env_aes_cbc_crypt(char *payload, const int enc) uint32_t aes_blocks;
/* First we expand the key. */
- aes_expand_key(common_args.aes_key, key_exp);
aes_expand_key(key, key_exp);
/* Calculate the number of AES blocks to encrypt. */ aes_blocks = DIV_ROUND_UP(len, AES_KEY_LENGTH);
@@ -1186,7 +1189,8 @@ int fw_env_open(void) crc0 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
if (common_args.aes_flag) {
ret = env_aes_cbc_crypt(environment.data, 0);
ret = env_aes_cbc_crypt(environment.data, 0,
if (ret) return ret; }common_args.aes_key);
@@ -1243,7 +1247,8 @@ int fw_env_open(void) crc1 = crc32 (0, (uint8_t *) redundant->data, ENV_SIZE);
if (common_args.aes_flag) {
ret = env_aes_cbc_crypt(redundant->data, 0);
ret = env_aes_cbc_crypt(redundant->data, 0,
}common_args.aes_key); if (ret) return ret;
Regards, Stefano

checkpatch complains about in succeding patch. Prefer to fix all declarations in a dedicated patch.
Signed-off-by: Andreas Fenkart andreas.fenkart@digitalstrom.com --- tools/env/fw_env.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h index 57149e7..7345922 100644 --- a/tools/env/fw_env.h +++ b/tools/env/fw_env.h @@ -78,12 +78,12 @@ 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[]); -extern int fw_parse_script(char *fname); -extern int fw_env_open(void); -extern int fw_env_write(char *name, char *value); -extern int fw_env_close(void); +int fw_printenv(int argc, char *argv[]); +char *fw_getenv(char *name); +int fw_setenv(int argc, char *argv[]); +int fw_parse_script(char *fname); +int fw_env_open(void); +int fw_env_write(char *name, char *value); +int fw_env_close(void);
-extern unsigned long crc32 (unsigned long, const unsigned char *, unsigned); +unsigned long crc32(unsigned long, const unsigned char *, unsigned);

Signed-off-by: Andreas Fenkart andreas.fenkart@digitalstrom.com --- tools/env/fw_env.c | 6 +++--- tools/env/fw_env.h | 4 ++-- tools/env/fw_env_main.c | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index c362a41..bd218cb 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -240,7 +240,7 @@ int parse_aes_key(char *key, uint8_t *bin_key) * Print the current definition of one, or more, or all * environment variables */ -int fw_printenv (int argc, char *argv[]) +int fw_printenv(int argc, char *argv[], int value_only) { char *env, *nxt; int i, rc = 0; @@ -263,7 +263,7 @@ int fw_printenv (int argc, char *argv[]) return 0; }
- if (printenv_args.name_suppress && argc != 1) { + if (value_only && argc != 1) { fprintf(stderr, "## Error: `-n' option requires exactly one argument\n"); return -1; @@ -284,7 +284,7 @@ int fw_printenv (int argc, char *argv[]) } val = envmatch (name, env); if (val) { - if (!printenv_args.name_suppress) { + if (!value_only) { fputs (name, stdout); putc ('=', stdout); } diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h index 7345922..d4daeea 100644 --- a/tools/env/fw_env.h +++ b/tools/env/fw_env.h @@ -67,7 +67,7 @@ struct common_args { extern struct common_args common_args;
struct printenv_args { - int name_suppress; + int value_only; }; extern struct printenv_args printenv_args;
@@ -78,7 +78,7 @@ extern struct setenv_args setenv_args;
int parse_aes_key(char *key, uint8_t *bin_key);
-int fw_printenv(int argc, char *argv[]); +int fw_printenv(int argc, char *argv[], int value_only); char *fw_getenv(char *name); int fw_setenv(int argc, char *argv[]); int fw_parse_script(char *fname); diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c index 3706d8f..2a45a0d 100644 --- a/tools/env/fw_env_main.c +++ b/tools/env/fw_env_main.c @@ -151,7 +151,7 @@ int parse_printenv_args(int argc, char *argv[]) EOF) { switch (c) { case 'n': - printenv_args.name_suppress = 1; + printenv_args.value_only = 1; break; case 'a': case 'c': @@ -240,7 +240,7 @@ int main(int argc, char *argv[]) }
if (do_printenv) { - if (fw_printenv(argc, argv) != 0) + if (fw_printenv(argc, argv, printenv_args.value_only)) retval = EXIT_FAILURE; } else { if (!setenv_args.script_file) {

for double buffering to work, redundant buffers must have equal size
Signed-off-by: Andreas Fenkart andreas.fenkart@digitalstrom.com --- tools/env/fw_env.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index bd218cb..3525563 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -75,7 +75,8 @@ static int dev_current;
#define CUR_ENVSIZE ENVSIZE(dev_current)
-#define ENV_SIZE getenvsize() +static unsigned long usable_envsize; +#define ENV_SIZE usable_envsize
struct env_image_single { uint32_t crc; /* CRC32 over data bytes */ @@ -125,18 +126,6 @@ static int parse_config (void); #if defined(CONFIG_FILE) static int get_config (char *); #endif -static inline ulong getenvsize (void) -{ - ulong rc = CUR_ENVSIZE - sizeof(uint32_t); - - if (HaveRedundEnv) - rc -= sizeof (char); - - if (common_args.aes_flag) - rc &= ~(AES_KEY_LENGTH - 1); - - return rc; -}
static char *skip_chars(char *s) { @@ -955,7 +944,7 @@ static int env_aes_cbc_crypt(char *payload, const int enc, uint8_t key[AES_KEY_LENGTH]) { uint8_t *data = (uint8_t *)payload; - const int len = getenvsize(); + const int len = usable_envsize; uint8_t key_exp[AES_EXPAND_KEY_LENGTH]; uint32_t aes_blocks;
@@ -1381,6 +1370,21 @@ static int parse_config () DEVNAME (1), strerror (errno)); return -1; } + + if (HaveRedundEnv && ENVSIZE(0) != ENVSIZE(1)) { + ENVSIZE(0) = ENVSIZE(1) = min(ENVSIZE(0), ENVSIZE(1)); + fprintf(stderr, + "Redundant environments have inequal size, set to 0x%08lx\n", + ENVSIZE(1)); + } + + usable_envsize = CUR_ENVSIZE - sizeof(uint32_t); + if (HaveRedundEnv) + usable_envsize -= sizeof(char); + + if (common_args.aes_flag) + usable_envsize &= ~(AES_KEY_LENGTH - 1); + return 0; }

Signed-off-by: Andreas Fenkart andreas.fenkart@digitalstrom.com --- tools/env/fw_env.c | 50 +++++++++++++++++++++++-------------------------- tools/env/fw_env.h | 25 +++++++------------------ tools/env/fw_env_main.c | 28 +++++++++++++++++---------- 3 files changed, 48 insertions(+), 55 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 3525563..9edefd7 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -35,10 +35,6 @@
#include "fw_env.h"
-struct common_args common_args; -struct printenv_args printenv_args; -struct setenv_args setenv_args; - #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
#define min(x, y) ({ \ @@ -121,7 +117,7 @@ static unsigned char obsolete_flag = 0;
static int flash_io (int mode); static char *envmatch (char * s1, char * s2); -static int parse_config (void); +static int parse_config(struct env_opts *opts);
#if defined(CONFIG_FILE) static int get_config (char *); @@ -229,12 +225,12 @@ int parse_aes_key(char *key, uint8_t *bin_key) * Print the current definition of one, or more, or all * environment variables */ -int fw_printenv(int argc, char *argv[], int value_only) +int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts) { char *env, *nxt; int i, rc = 0;
- if (fw_env_open()) + if (fw_env_open(opts)) return -1;
if (argc == 0) { /* Print all env variables */ @@ -290,12 +286,13 @@ int fw_printenv(int argc, char *argv[], int value_only) return rc; }
-int fw_env_close(void) +int fw_env_close(struct env_opts *opts) { int ret; - if (common_args.aes_flag) { + + if (opts->aes_flag) { ret = env_aes_cbc_crypt(environment.data, 1, - common_args.aes_key); + opts->aes_key); if (ret) { fprintf(stderr, "Error: can't encrypt env for flash\n"); @@ -448,7 +445,7 @@ int fw_env_write(char *name, char *value) * modified or deleted * */ -int fw_setenv(int argc, char *argv[]) +int fw_setenv(int argc, char *argv[], struct env_opts *opts) { int i; size_t len; @@ -462,7 +459,7 @@ int fw_setenv(int argc, char *argv[]) return -1; }
- if (fw_env_open()) { + if (fw_env_open(opts)) { fprintf(stderr, "Error: environment not initialized\n"); return -1; } @@ -498,7 +495,7 @@ int fw_setenv(int argc, char *argv[])
free(value);
- return fw_env_close(); + return fw_env_close(opts); }
/* @@ -518,7 +515,7 @@ int fw_setenv(int argc, char *argv[]) * 0 - OK * -1 - Error */ -int fw_parse_script(char *fname) +int fw_parse_script(char *fname, struct env_opts *opts) { FILE *fp; char dump[1024]; /* Maximum line length in the file */ @@ -528,7 +525,7 @@ int fw_parse_script(char *fname) int len; int ret = 0;
- if (fw_env_open()) { + if (fw_env_open(opts)) { fprintf(stderr, "Error: environment not initialized\n"); return -1; } @@ -616,10 +613,9 @@ int fw_parse_script(char *fname) if (strcmp(fname, "-") != 0) fclose(fp);
- ret |= fw_env_close(); + ret |= fw_env_close(opts);
return ret; - }
/* @@ -1130,7 +1126,7 @@ static char *envmatch (char * s1, char * s2) /* * Prevent confusion if running from erased flash memory */ -int fw_env_open(void) +int fw_env_open(struct env_opts *opts) { int crc0, crc0_ok; unsigned char flag0; @@ -1145,7 +1141,7 @@ int fw_env_open(void) struct env_image_single *single; struct env_image_redundant *redundant;
- if (parse_config ()) /* should fill envdevices */ + if (parse_config(opts)) /* should fill envdevices */ return -1;
addr0 = calloc(1, CUR_ENVSIZE); @@ -1177,9 +1173,9 @@ int fw_env_open(void)
crc0 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
- if (common_args.aes_flag) { + if (opts->aes_flag) { ret = env_aes_cbc_crypt(environment.data, 0, - common_args.aes_key); + opts->aes_key); if (ret) return ret; } @@ -1235,9 +1231,9 @@ int fw_env_open(void)
crc1 = crc32 (0, (uint8_t *) redundant->data, ENV_SIZE);
- if (common_args.aes_flag) { + if (opts->aes_flag) { ret = env_aes_cbc_crypt(redundant->data, 0, - common_args.aes_key); + opts->aes_key); if (ret) return ret; } @@ -1314,15 +1310,15 @@ int fw_env_open(void) }
-static int parse_config () +static int parse_config(struct env_opts *opts) { struct stat st;
#if defined(CONFIG_FILE) /* Fills in DEVNAME(), ENVSIZE(), DEVESIZE(). Or don't. */ - if (get_config(common_args.config_file)) { + if (get_config(opts->config_file)) { fprintf(stderr, "Cannot parse config file '%s': %m\n", - common_args.config_file); + opts->config_file); return -1; } #else @@ -1382,7 +1378,7 @@ static int parse_config () if (HaveRedundEnv) usable_envsize -= sizeof(char);
- if (common_args.aes_flag) + if (opts->aes_flag) usable_envsize &= ~(AES_KEY_LENGTH - 1);
return 0; diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h index d4daeea..dac964d 100644 --- a/tools/env/fw_env.h +++ b/tools/env/fw_env.h @@ -57,33 +57,22 @@ "bootm" #endif
-struct common_args { +struct env_opts { #ifdef CONFIG_FILE char *config_file; #endif - uint8_t aes_key[AES_KEY_LENGTH]; int aes_flag; /* Is AES encryption used? */ + uint8_t aes_key[AES_KEY_LENGTH]; }; -extern struct common_args common_args; - -struct printenv_args { - int value_only; -}; -extern struct printenv_args printenv_args; - -struct setenv_args { - char *script_file; -}; -extern struct setenv_args setenv_args;
int parse_aes_key(char *key, uint8_t *bin_key);
-int fw_printenv(int argc, char *argv[], int value_only); +int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts); char *fw_getenv(char *name); -int fw_setenv(int argc, char *argv[]); -int fw_parse_script(char *fname); -int fw_env_open(void); +int fw_setenv(int argc, char *argv[], struct env_opts *opts); +int fw_parse_script(char *fname, struct env_opts *opts); +int fw_env_open(struct env_opts *opts); int fw_env_write(char *name, char *value); -int fw_env_close(void); +int fw_env_close(struct env_opts *opts);
unsigned long crc32(unsigned long, const unsigned char *, unsigned); diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c index 2a45a0d..7a17b28 100644 --- a/tools/env/fw_env_main.c +++ b/tools/env/fw_env_main.c @@ -49,6 +49,14 @@ static struct option long_options[] = { {NULL, 0, NULL, 0} };
+static struct env_opts env_opts; + +/* setenv options */ +static int noheader; + +/* getenv options */ +static char *script_file; + void usage_printenv(void) {
@@ -108,22 +116,22 @@ static void parse_common_args(int argc, char *argv[]) int c;
#ifdef CONFIG_FILE - common_args.config_file = CONFIG_FILE; + env_opts.config_file = CONFIG_FILE; #endif
while ((c = getopt_long(argc, argv, ":a:c:h", long_options, NULL)) != EOF) { switch (c) { case 'a': - if (parse_aes_key(optarg, common_args.aes_key)) { + if (parse_aes_key(optarg, env_opts.aes_key)) { fprintf(stderr, "AES key parse error\n"); exit(EXIT_FAILURE); } - common_args.aes_flag = 1; + env_opts.aes_flag = 1; break; #ifdef CONFIG_FILE case 'c': - common_args.config_file = optarg; + env_opts.config_file = optarg; break; #endif case 'h': @@ -151,7 +159,7 @@ int parse_printenv_args(int argc, char *argv[]) EOF) { switch (c) { case 'n': - printenv_args.value_only = 1; + noheader = 1; break; case 'a': case 'c': @@ -177,7 +185,7 @@ int parse_setenv_args(int argc, char *argv[]) EOF) { switch (c) { case 's': - setenv_args.script_file = optarg; + script_file = optarg; break; case 'a': case 'c': @@ -240,14 +248,14 @@ int main(int argc, char *argv[]) }
if (do_printenv) { - if (fw_printenv(argc, argv, printenv_args.value_only)) + if (fw_printenv(argc, argv, noheader, &env_opts) != 0) retval = EXIT_FAILURE; } else { - if (!setenv_args.script_file) { - if (fw_setenv(argc, argv) != 0) + if (!script_file) { + if (fw_setenv(argc, argv, &env_opts) != 0) retval = EXIT_FAILURE; } else { - if (fw_parse_script(setenv_args.script_file) != 0) + if (fw_parse_script(script_file, &env_opts) != 0) retval = EXIT_FAILURE; } }

On Tue, Apr 05, 2016 at 11:13:42PM +0200, Andreas Fenkart wrote:
Signed-off-by: Andreas Fenkart andreas.fenkart@digitalstrom.com
Applied to u-boot/master, thanks!

Hi Andreas,
On 05/04/2016 23:13, Andreas Fenkart wrote:
u-boot tools can be built as a library (ibubootenv.a). Passing arguments to the library using global structures is not a good interface.
Andreas Fenkart (5): tools/env: make env_aes_cbc_crypt re-entrant tools/env: remove 'extern' from function prototype in fw_env.h tools/env: fw_printenv pass value_only as argument tools/env: compute size of usable area only once tools/env: no global variable sharing between application and library
tools/env/fw_env.c | 95 ++++++++++++++++++++++++++----------------------- tools/env/fw_env.h | 31 ++++++---------- tools/env/fw_env_main.c | 28 +++++++++------ 3 files changed, 78 insertions(+), 76 deletions(-)
I can confirm that series fixes building an external program using the env tools as environment. Thanks for fixing it !
Best regards, Stefano Babic
participants (3)
-
Andreas Fenkart
-
Stefano Babic
-
Tom Rini