Dear Robin Getz,
In message <[email protected]> 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: [email protected]
Question: How does one get fresh air into a Russian church?
Answer: One clicks on an icon, and a window opens!
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
