On Sun, 31 Aug 2008, Wolfgang Denk wrote:
> 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.
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: [EMAIL PROTECTED]
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
