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

[Robin Getz]

On Mon 17 Aug 2009 15:05, Wolfgang Denk pondered:
> Dear Robin Getz,
> 
> In message <200908171315.40365.rgetz at 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).