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

[Wolfgang Denk]

Dear Timur,

in message <11782314361072-git-send-email-timur at 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