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

[Guennadi Liakhovetski]

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

> 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.

This union replaces the typedef env_t, which was also defined in fw_env.c. 
Thus I was not fixing the issue you describe above, which I fully agree 
with - the tool and u-boot should ideally use the same definition from a 
common header. I just did not address this issue in this patch series.

> 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").

Would be good, yes, the question is - do we want to do this now and do we 
want to do this in the scope of this patch series?

> > +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.

My understanding until now, that U-Boot follows the same coding style as 
the Linux kernel with only one exception - a space between a function name 
and the opening parenthesis. This is also stated here:

http://www.denx.de/wiki/U-Boot/CodingStyle

and Linux explicitly discourages typedef. They also produce warnings by 
checkpatch.sh. If this is also different in U-Boot, no problem, can change 
back.

> > -	/* 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.

I think, CRC is always at offset 0, then follows the (optional) flag byte, 
and then comes the data. I also see this with binary dumps of the 
environment. Otherwise what takes the first four bytes? Or did you mean 
the data which CRC is calculated is at offset 4 or 5? I think, the comment 
is correct.

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

That's exactly the reason - the comment explains, that the crc is at the 
same location, so, you can access it using single.crc in both cases.

> 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.

I need it to be able to copy the whole environment image, including the 
crc and the optional flag with one read / write / memcpy operation. And 
this is also an advantage even on NOR - no need for two or three syscalls, 
the whole image is read / written with one syscall.

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 at denx.de