Dear Timur,
in message 463A88BA.2030607@freescale.com you wrote:
So before your patch we had "len", "checksum", and "data", and now we have "len", "data", and "checksum".
What is your reason for this change? I see no functinal difference nor any improvement of readability.
It's for the comment that I added. I wanted to make sure that the reader understood what 'len' and 'data' are for, since their purpose is not obvious from just reading the code.
Your comment is actually wrong. The use of "len" is not limited to that purpose.
But that doesn't mean that you can overwrite this variable without checking.
if (crc32 (0, (uchar *)data, len) != checksum) {
if (crc32 (0, (uchar *) &header, sizeof(image_header_t)) != checksum) {This looks functionally identical to me, but is less readable.
The point behind the patch is to make sure that 'len' and 'data' are only used for the purpose they are intended. Here, len and data are used as temporary variables.
...and here this is perfectly legal, as "len" does not store any other value that is needed later.
@@ -779,9 +776,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
checksum = ntohl(hdr->ih_dcrc); addr = (ulong)((uchar *)(hdr) + sizeof(image_header_t));
len = ntohl(hdr->ih_size);if(checksum != crc32(0, (uchar *)addr, len)) {
if(checksum != crc32(0, (uchar *)addr, ntohl(hdr->ih_size))) {Same here. functionally identical but less readable.
Not true. In this case, len has already been assigned the length of the initrd, and here we lose this value. Without this change, the kernel will panic at boot.
Right. I agree that this is the core of the problem, but from your patch it was impossible to see. You have to read the diff, plus your patch description, plus the code.
I think we should just fix the bug, which is the incorrect use of the variable "len" at a place where it was already used. As Johns Daniel pointed out (see his message from Tue, 27 Mar 2007 11:25:55 -0500), this is the core of your poatch and all that needs to be changed.
With this patch it is impossible to see what the problem is you are trying to fix.
It's clearly explained in the comment: 'data' and 'len' are the address and length of the initrd, so this patch makes sure that these variables only contain those values.
No, this explanation is wrong. "len" gets used for some other purposes, too.
To put this problem finally to rest I checked in the fix as suggested by Johns Daniel, i. e. the core idea of your patch. See http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commit;h=9877d7dcd1eebe...
Thanks for pointing out the problem and providing a fix.
And please accept my apologies thatt his was so complicated and took so long. [Nevertheless you still might want to try to find a way to access the repository I created for you in case you have more patches.]
Best regards,
Wolfgang Denk