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