[U-Boot] [PATCH] - add dns

[Wolfgang Denk]

Dear Robin Getz,

In message <200907171745.36176.rgetz at blackfin.uclinux.org> you wrote:
>
> > You probably should add a doc/README.* file to explain how that is
> > supposed to be used.
> 
> Will do.
> 
> What is the rule for when things go in ./doc/README.* vs ./README ?

Size - anything that takes more than 5...10 lines ?

> > Could you generate a git patch instead?
> 
> Sure - I'll generate something from the net tree?

Please use the mainline repo / "master" branch as reference.

> > > +#if defined(CONFIG_CMD_DNS)
> > > +extern char NetDNSResolve[255];			/* The host to resolve  */
> > > +#endif
> > 
> > Can this buffer overflow?
> 
> Shouldn't.

Agreed. I've seen the tests later in the code but forgot to remove
this.

...
> > Hmmm... why do you assume that the address we're trying to resolve is
> > the server IP?
> > 
> > To me it makes at least as much sense to resolve the "ipaddr" value.
> 
> Yeah, this was my biggest issue for things too (from the original patch).
> 
> but you can easily :
> 
> set nameip ${serverip}
> 
> to transfer it to something else. - or just use it directly.

But you lost the original "serverip" setting, which may not be wanted.

> > > +	*p++ = 0;			/* Mark end of host name */
> > > +	*p++ = 0;			/* Well, lets put this byte as well */
> > 
> > Why that?
> 
> No idea - that was in the original....
> 
> Hmm -- according to my TCP/IP Illustrated - names are suppost to be double
> null terminated - so I think that is a requirement.

Then please remove the comment that automatically triggers such "why
that?" questions (and eventually replace it with a better
explanation).


> > I suggest to change this as follows:
...
> > What do you think?
> 
> that is more flexible that the current implementation...

Indeed :-)

> but also a little more complex.

Not really. Yes, you have to add 3 lines of code to check for the 2nd
argument, butexcept from that you just change the 

	setenv("serverip", IPStr);
into
	setenv(argvp, IPStr);

> saving it in a predefined env var (like 'dnshostip') is also OK.
> 
> dns www.denx.de
> set serverip ${dnshostip}
> 
> -- is going to be a little smaller...
> 
> Up to you...

I like my proposal better, as it avoids adding another predefined env
var which is of not much use to most users, as it will only serve  as
temp storage; and your proposal requires a second command, i. e. more
typing and thus more chances for typos.

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
The Wright Bothers weren't the first to fly. They were just the first
not to crash.