[U-Boot] [PATCH 0/4] Network defrag

[Wolfgang Denk]

Dear Robin Getz,

In message <200908171315.40365.rgetz at blackfin.uclinux.org> you wrote:
>
> Comments welcome...

I guess the code is largely untested?

> diff --git a/net/tftp.c b/net/tftp.c
> index 9544691..56db247 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -66,6 +66,9 @@ static ulong	TftpLastBlock;		/* last packet sequence number received */
>  static ulong	TftpBlockWrap;		/* count of sequence number wraparounds */
>  static ulong	TftpBlockWrapOffset;	/* memory offset due to wrapping	*/
>  static int	TftpState;
> +#ifdef CONFIG_TFTP_TSIZE
> +static int	TftpTsize;		/* Tsize */
> +#endif

Why static int? This gives a random init value for the second and each
following TFTP transfers.

> @@ -212,6 +215,10 @@ TftpSend (void)
>  		sprintf((char *)pkt, "%lu", TIMEOUT / 1000);
>  		debug("send option \"timeout %s\"\n", (char *)pkt);
>  		pkt += strlen((char *)pkt) + 1;
> +#ifdef CONFIG_TFTP_TSIZE
> +		pkt += sprintf((char *)pkt,"tsize%c%d", 0,0);
> +		pkt += strlen((char *)pkt) + 1;

Looks to me as if you were adding the length twice?

> +#endif
>  		/* try for more effic. blk size */
>  		pkt += sprintf((char *)pkt,"blksize%c%d%c",
>  				0,TftpBlkSizeOption,0);
> @@ -321,8 +328,14 @@ TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len)
>  					simple_strtoul((char*)pkt+i+8,NULL,10);
>  				debug("Blocksize ack: %s, %d\n",
>  					(char*)pkt+i+8,TftpBlkSize);
> -				break;
>  			}
> +#ifdef CONFIG_TFTP_TSIZE
> +			if (strcmp ((char*)pkt+i,"tsize") == 0) {
> +				TftpTsize = simple_strtoul((char*)pkt+i+6,NULL,10);
> +				debug("size = %s, %d\n",
> +					 (char*)pkt+i+6, TftpTsize);
> +			}

It seems you rely on TftpTsize to be zero otherwise. The current code
(with a static int) does not guarantee that, though.

> -		} else {
> +		}
> +#ifdef CONFIG_TFTP_TSIZE
> +		else if (TftpTsize) {
> +
> +			printf("%2d\b\b", NetBootFileXferSize * 100 / TftpTsize);
> +		}
> +#endif

Hm... maybe we should rather print hashes (say one '#' for each 2%,
resulting in a maximum of 50 characters output.

Otherwise we actually increase the amount of characters sent over the
serial line (and the intention was to reduce it, right?)

> +#ifdef CONFIG_TFTP_TSIZE
> +			if (TftpTsize)
> +				puts("100%");
> +#endif
> +
>  			puts_quiet("\ndone\n");

Here we see the bad consequences of this puts_quiet() stuff (which I
really dislike. The longer I see it, the more I tend to reject it.

Here, the (necessary!) newline terminating the "100%" output, is not
always sent, only when puts_quiet() is "active".

> @@ -550,6 +579,9 @@ TftpStart (void)
>  #ifdef CONFIG_TFTP_TIME
>  	start_time = get_timer(0);
>  #endif
> +#ifdef CONFIG_TFTP_TSIZE
> +	TftpTsize = 0;
> +#endif

Ah. I see :-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: 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
Question: How does one get fresh air into a Russian church?
Answer:   One clicks on an icon, and a window opens!