On Mon 17 Aug 2009 15:05, Wolfgang Denk pondered:
> Dear Robin Getz,
>
> In message <200908171315.40365.rg...@blackfin.uclinux.org> you wrote:
> >
> > Comments welcome...
>
> I guess the code is largely untested?
I tested it on a single machine.
> > 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.
Nope - it is set to zero on the start of every transfer.
> > @@ -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?
One zero is for the null char (delimiter), one zero is for ascii zero (length).
> > +#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.
It is set to zero below on start.
> > - } 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?)
Yeah, it was to reduce the output - this was easier :)
Plus spinning numbers are always nice. When you are doing a scp - do you
look at the bar moving, or the numbers going up to 100%? I always look
at the numbers...
I'll re-work it for a single line of 50 hashes. (one '#' == 2% of the file).
> > +#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".
Yeah, sorry about that - I would switch the puts to the puts_quiet
if that is picked up - I just never heard an ack or nack on it...
> > @@ -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 :-)
So - you are OK with they way it is done (other comments still apply of
course).
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
