tools/env: pass arguments as parameters not global structure
cover 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(-)
Signature · 1 line
2.8.0.rc3
PATCH 1/5 tools/env: make env_aes_cbc_crypt re-entrant
--- tools/env/fw_env.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
tools/env/fw_env.c +11 -6
@@ -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; }
Signature · 1 line
2.8.0.rc3
PATCH 1/5 tools/env: make env_aes_cbc_crypt re-entrant
Hi Andreas,
thanks for fixing this:
Quoted 10 lines · Andreas Fenkart
Signed-off-by Andreas Fenkart <[email protected]>--- tools/env/fw_env.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)tools/env/fw_env.c +0
-0
tools/env/fw_env.c static struct environment environment = { · line 111
Quoted 7 lines
tools/env/fw_env.c +2
-1@@ -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);
Quoted 13 lines
tools/env/fw_env.c +2
-1static 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" );
tools/env/fw_env.c static int flash_flag_obsolete (int dev, int fd, off_t offset) · line 955
Quoted 6 lines
tools/env/fw_env.c +2
-1@@ -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.
Quoted 24 lines
tools/env/fw_env.c +3
-2{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; }
tools/env/fw_env.c int fw_env_open(void) · line 1254
Quoted 9 lines
tools/env/fw_env.c +2
-1@@ -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
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================
PATCH 2/5 tools/env: remove 'extern' from function prototype in fw_env.h
In reply to Andreas Fenkartcheckpatch complains about in succeding patch. Prefer to fix all declarations in a dedicated patch.
--- tools/env/fw_env.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
tools/env/fw_env.h +8 -8
@@ -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 );
Signature · 1 line
2.8.0.rc3
PATCH 3/5 tools/env: fw_printenv pass value_only as argument
In reply to Andreas Fenkart--- 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(-)
tools/env/fw_env.c +3 -3
@@ -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); }
tools/env/fw_env.h +2 -2
@@ -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);
tools/env/fw_env_main.c +2 -2
@@ -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) {
Signature · 1 line
2.8.0.rc3
PATCH 4/5 tools/env: compute size of usable area only once
In reply to Andreas Fenkartfor double buffering to work, redundant buffers must have equal size
--- tools/env/fw_env.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
tools/env/fw_env.c +18 -14
@@ -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_envsizestruct 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 ; }
Signature · 1 line
2.8.0.rc3
PATCH 5/5 tools/env: no global variable sharing between application and library
In reply to Andreas Fenkart--- 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(-)
tools/env/fw_env.c +23 -27
@@ -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 ;
tools/env/fw_env.h +7 -18
@@ -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 );
tools/env/fw_env_main.c +18 -10
@@ -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 ; } }
Signature · 1 line
2.8.0.rc3
PATCH 5/5 tools/env: no global variable sharing between application and library
Quoted 1 line · Andreas Fenkart
Signed-off-by Andreas Fenkart <[email protected]>
Applied to u-boot/master, thanks!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160530/8ede0673/attachment.sig>
cover tools/env: pass arguments as parameters not global structure
In reply to Andreas FenkartHi Andreas,
Quoted 10 lines · Andreas Fenkart
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: makeenv_aes_cbc_cryptre-entrant
tools/env: remove 'extern' from function prototype infw_env.htools/env:fw_printenvpassvalue_onlyas argumenttools/env: compute size of usable area only oncetools/env: no global variable sharing between application and library
Quoted 5 lines
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
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================