Re: [U-Boot] [PATCH 5/5] New implementation for internal handling of environment variables.

Hi Wolfgang,

I could think of some situations where the new env command is helpful. But more during development than for production systems.

Switching between environment profiles would be cool. And a "env default -f" behavior that keeps MAC addresses and serial# is also on my wishlist.

I did some testing on our PMC440 with environment in EEPROM. Please see some comments below.

On Saturday 17 July 2010 21:45, Wolfgang Denk wrote:

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index 8c86f15..976a30b 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c

...

+/*

• • env export [-t | -b | -c] addr [size]
• • -t: export as text format; if size is given, data will be

• • padded with '\0' bytes; if not, one terminating '\0'
    

• • will be added (which is included in the "filesize"
    

• • setting so you can for exmple copy this to flash and
    

• • keep the termination).
    

• • -b: export as binary format (name=value pairs separated by

• • '\0', list end marked by double "\0\0")
    

• • -c: export as checksum protected environment format as

• • used for example by "saveenv" command
    

• • addr: memory address where environment gets stored
• • size: size of output buffer

...

+static int do_env_export(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{

• char buf[32];
• char *addr, *cmd, *res;
• size_t size;
• ssize_t len;
• env_t *envp = (env_t *)addr;

addr is uninitialized. declaration is enough here.

• char sep = '\n';
• int chk = 0;
• int fmt = 0;
• cmd = *argv;
• while (--argc > 0 && **++argv == '-') {

I'd like some more braces for readability.

• char *arg = *argv;
  

• while (*++arg) {
  

• 	switch (*arg) {
  

• 	case 'b':		/* raw binary format */
  

• 		if (fmt++)
  

• 			goto sep_err;
  

• 		sep = '\0';
  

• 		break;
  

• 	case 'c':		/* external checksum format */
  

• 		if (fmt++)
  

• 			goto sep_err;
  

• 		sep = '\0';
  

• 		chk = 1;
  

• 		break;
  

• 	case 't':		/* text format */
  

• 		if (fmt++)
  

• 			goto sep_err;
  

• 		sep = '\n';
  

• 		break;
  

• 	default:
  

• 		cmd_usage(cmdtp);
  

• 		return 1;
  

• 	}
  

• }
  

• }

-/**************************************************/

• if (argc < 1) {

• cmd_usage(cmdtp);
  

• return 1;
  

• }
• addr = (char *)simple_strtoul(argv[0], NULL, 16);
• if (argc == 2) {

• size = simple_strtoul(argv[1], NULL, 16);
  

• memset(addr, '\0', size);
  

• } else {

• size = 0;
  

• }
• if (sep) { /* export as text file */

• len = hexport(sep, &addr, size);
  

• if (len < 0) {
  

• 	error("Cannot export environment: errno = %d\n",
  

• 		errno);
  

• 	return 1;
  

• }
  

• sprintf(buf, "%zX", len);
  

• setenv("filesize", buf);
  

• return 0;
  

• }
• if (chk) { /* export as checksum protected block */

Add: envp = (env_t *)addr;

• res = (char *)&envp->data;
  

• } else { /* export as raw binary data */

• res = (char *)&addr;
  

Should'n this be res = addr;

• }
• len = hexport('\0', &res, ENV_SIZE);
• if (len < 0) {

• error("Cannot export environment: errno = %d\n",
  

• 	errno);
  

• return 1;
  

• }
• if (chk) {

• envp->crc   = crc32(0, envp->data, ENV_SIZE);
  

+#ifdef CONFIG_ENV_ADDR_REDUND

• envp->flags = ACTIVE_FLAG;
  

+#endif

• }
• sprintf(buf, "%zX", len + offsetof(env_t,data));
• setenv("filesize", buf);
• return 0;

+sep_err:

• printf("## %s: only one of "-b", "-c" or "-t" allowed\n",

• cmd);
  

• return 1;

+}

...

Fixes for non-building board (AR405, CANBT, PMC440) will come up shortly.

Matthias