Dear Guennadi Liakhovetski,
In message <[EMAIL PROTECTED]> 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
--
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: [EMAIL PROTECTED]
If A equals success, then the formula is A = X + Y + Z. X is work. Y
is play. Z is keep your mouth shut. - Albert Einstein
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
