On Sun, 31 Aug 2008, Wolfgang Denk wrote:
Dear Guennadi Liakhovetski,
In message Pine.LNX.4.64.0808271745000.6718@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@denx.de