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

[Wolfgang Denk <wd@denx.de>]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0808311740070.3747@axis700.grange> you wrote:
> 
> 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.

This is not correct. You are creating this issue. So far, U-Boot and
fw_env used to use the same (or at least equivalent) definitions.

And I have to admit that I'm not a friend of using unions. They are a
great way to obfuscate code.

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

If there is no real need to change it now, then leave it unchanged?

> > Please stick with the typedef.
...
> 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.

Ah! And why didn't you mention this in the patch?

For a reviewer it is very important to understand why you are
modifying code.


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

Yes, you are right.

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

And we see the obfuscation caused by using a union.

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

This is ugly, plain ugly.

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

Hm... why do you need to do this in a single read operation?  Where's
the  difference  between reading it all at once or in several smaller
chunks?

> this is also an advantage even on NOR - no need for two or three syscalls, 
> the whole image is read / written with one syscall.

A big win, really ;-)

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
How long does it take a  DEC  field  service  engineer  to  change  a
lightbulb?       It depends on how many bad ones he brought with him.