[U-Boot-Users] [PATCH v3] Fix initrd length miscalculation in bootm command when using a dtu

Timur Tabi
2007-05-04T01:13:30Z

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

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

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

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

--
Timur Tabi
Linux Kernel Developer @ Freescale