[U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0808271745000.6718 at axis700.grange> you wrote:
> Use a union to cover both with and without redundant environment cases.
...
> -typedef struct environment_s {
> -	ulong crc;			/* CRC32 over data bytes    */
> -	unsigned char flags;		/* active or obsolete */
> -	char *data;
> -} env_t;
> +/* This union will occupy exactly CFG_ENV_SIZE bytes. */
> +union env_image {
> +	struct {
> +		uint32_t	crc;	/* CRC32 over data bytes    */
> +		char		data[];
> +	} single;
> +	struct {
> +		uint32_t	crc;	/* CRC32 over data bytes    */
> +		unsigned char	flags;	/* active or obsolete */
> +		char		data[];
> +	} redund;
> +};

Hm... You defione this union in the  context  of  tools/env/fw_env.c,
while  "include/environment.h" uses a different typedef. I think this
is  extremly  error-prone  because   the   connection   between   the
environment-handling  code  in  U-Boot  and that in the external tool
gets lost.

I think both sets of functions should use the same set of definitions
(yes, I am aware that  this  requires  chnages  to  "include/environ-
ment.h").

> +struct environment {
> +	union env_image	*image;
> +	char		*data;	/* shortcut to data */
> +};
>  
> -static env_t environment;
> +static struct environment environment;

Omitting the typedef and then changing "env_t" into "struct
environment" makes no sense to me. It just makes for less readable
code and more typing.

Please stick with the typedef.

>  static int HaveRedundEnv = 0;
>  
> +#define ENV_FLAGS(e) e.image->redund.flags
> +
>  static unsigned char active_flag = 1;
>  static unsigned char obsolete_flag = 0;
>  
> @@ -156,7 +170,7 @@ static char default_environment[] = {
>  #ifdef  CONFIG_EXTRA_ENV_SETTINGS
>  	CONFIG_EXTRA_ENV_SETTINGS
>  #endif
> -	"\0"			/* Termimate env_t data with 2 NULs */
> +	"\0"			/* Termimate struct environment data with 2 NULs */
>  };
>  
>  static int flash_io (int mode);
> @@ -382,8 +396,12 @@ int fw_setenv (int argc, char *argv[])
>  
>    WRITE_FLASH:
>  
> -	/* Update CRC */
> -	environment.crc = crc32 (0, (uint8_t*) environment.data, ENV_SIZE);
> +	/*
> +	 * Update CRC: it is at the same location with and without the
> +	 * redundant environment
> +	 */
> +	environment.image->single.crc = crc32 (0, (uint8_t *) environment.data,
> +					       ENV_SIZE);

The comment is wrong. Without redundant environment, the CRC is at
offset 4 from the start of the environment storage, and with
redundancy it's at offset 5. This is definitely not the same location.

Also, the code does not match the commend, since you access
"single.crc" which has no relation to redundant environment
at all.


Looking at the rest of the code I see no  improvement  in  using  the
union  versus  what  we did before - the code is even longer now, and
(slightly) less readable to me.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If A equals success, then the formula is A = X + Y + Z. X is work.  Y
is play. Z is keep your mouth shut.                 - Albert Einstein