Dear Robin Getz,
In message <200907171745.36176.rg...@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: w...@denx.de
The Wright Bothers weren't the first to fly. They were just the first
not to crash.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
