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