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

Wolfgang Denk
2007-05-03T23:47:27Z

Dear Timur,

in message <11782314361072-git-send-email-timur@freescale.com> you wrote:
> The do_bootm_linux() function uses the same variable ('len') to calculate the
> dtu (flat device tree wrapped in a uImage) length and the initrd length, which
> means that the initrd length was incorrect when it was needed. This patch
> changes any code that uses 'len' or 'data' as temporary variables to use
> the values directly instead. It also adds a comment to the definitions
> of 'len' and 'data' reminding other programmers what they represent.
...
> This patch is a simplied version of the initrd patch that only fixes the root
> problem, without renaming any variables. This bug was introduced in commit
> 87a449c8ac396420cb24260f717ea9e6faa82047.
...
> @@ -523,11 +523,11 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
> int verify)
> {
> ulong sp;
> - ulong len, checksum;
> + ulong len, data; /* The initrd length and address */
> ulong initrd_start, initrd_end;
> ulong cmd_start, cmd_end;
> ulong initrd_high;
> - ulong data;
> + ulong checksum;
> int initrd_copy_to_ram = 1;
> char *cmdline;
> char *s;

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.

> @@ -652,13 +652,10 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
> do_reset (cmdtp, flag, argc, argv);
> }
>
> - data = (ulong)&header;
> - len = sizeof(image_header_t);
> -
> checksum = ntohl(hdr->ih_hcrc);
> hdr->ih_hcrc = 0;
>
> - 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.

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

With this patch it is impossible to see what the problem is you are
trying to fix.

Looking at the patch, I see only cosmetical changes, which actually
lead to less readable code.

If there is a problem, please fix the problem, but don't touch
innocent code that has been there for ages.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, CEO: 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: wd at denx.de
I think there's a world market for about five computers.
-- attr. Thomas J. Watson (Chairman of the Board, IBM), 1943