Re: [U-Boot] [PATCH] fw_env: add NAND support

On Tue, 2 Sep 2008, Wolfgang Denk wrote:

...

@@ -596,107 +844,120 @@ static char *envmatch (char * s1, char * s2) static int env_init (void) { int crc1, crc1_ok;

• char flag1, *addr1;

  int crc2, crc2_ok;

• char flag2, *addr2;

I think you should number these 0 an 1, respectively here.

Because then you can read this code much better:

...

if (crc1_ok && !crc2_ok) {

• 	dev_current = 0;
  

  } else if (!crc1_ok && crc2_ok) {

• 	dev_current = 1;
  

would become:

if (crc0_ok && !crc1_ok) { 
	dev_current = 0;
} else if (!crc0_ok && crc1_ok) {
	dev_current = 1;

If "0" is OK, then use "0"; if "1" is ok then use "1".

Your version: if "1" is OK then use "0", if "2" is OK then use "1" is more difficult to read.

[Or stick with the old identifiers, i. e. use 1 and 2 consistently, but don't mix 0/1 here and 1/2 there.]

Sorry, don't understand. This is the original code:

if (crc1_ok && !crc2_ok) { environment.data = addr1; environment.flags = flag1; environment.crc = crc1; curdev = 0; free (addr2);

Which looks to me like "if (crc1_ok...) {... curdev = 0;...}", which I tried to preserve. So, you would prefer me to _change_ it?

...

• if ((fp = fopen (fname, "r")) == NULL)

• return -1;
  
  

• while (i < 2 && fgets (dump, sizeof (dump), fp)) { /* Skip incomplete conversions and comment strings */

• if (dump[0] == '#')
  continue;
  

I think you must initialize ENVSECTORS(i) here...

...

• rc = sscanf (dump, "%s %lx %lx %lx %lx",
  

• 	     DEVNAME (i),
  

• 	     &DEVOFFSET (i),
  

• 	     &ENVSIZE (i),
  

• 	     &DEVESIZE (i),
  

• 	     &ENVSECTORS (i));
  

• if (rc < 4)
  

• 	continue;
  

• if (rc < 5)
  

• 	/* Default - 1 sector */
  

• 	ENVSECTORS (i) = 1;
  

Because if rc < 4, you already continued and left ENVSECTORS uninitialized.

As far as I understand, with rc == 3 there is no DEVESIZE in the line, which doesn't look like a valid configuration line to me. The orginal code however accepted such lines and only dropped lines with rc < 3. I do not understand how the original code managed to work with rc == 3. As I thought this was a bug, I changed the test to "rc < 4", i.e., now I require at least 4 fields, in which case I initialise ENVSECTORS to the default value - 1 sector. If rc < 4 the counter "i" is not incremented and the line is dropped - in the same way as in the original version. Or am I missing something?

Thanks Guennadi --- Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de