On Fri 17 Jul 2009 16:55, Wolfgang Denk pondered:
> Dear Robin Getz,
>
> In message <200907171553.08108.rg...@blackfin.uclinux.org> you wrote:
> > On 04 Oct 2008 Pieter posted a dns implementation for U-Boot.
> >
> > http://www.mail-archive.com/u-boot-us...@lists.sourceforge.net/msg10216.html
> >
> > > DNS can be enabled by setting CFG_CMD_DNS. After performing a query, the
> > > serverip environment var is updated.
> > >
> > > Probably there are some cosmetic issues with the patch. Unfortunatly I do
> > > not have the time to correct these. So if anybody else likes DNS support
> > > in
> > > U-Boot and has the time, feel free to patch it in the main tree.
> >
> > Here it is again - slightly modified & smaller:
> > - update to 2009-06 (Pieter's patch was for U-Boot 1.2.0)
> > - run through checkpatch, and clean up style issues
> > - remove packet from stack
> > - cleaned up some comments
> > - failure returns much faster (if server responds, don't wait for timeout)
> > - use built in functions (memcpy) rather than byte copy.
> >
> > bfin> dhcp
> > BOOTP broadcast 1
> > DHCP client bound to address 192.168.0.4
> > bfin> dns pool.ntp.org
> > 69.36.241.112
> > bfin> sntp $(serverip)
> > Date: 2009-07-17 Time: 19:16:51
> > bfin> dns www.google.com
> > 64.233.161.147
> > bfin> ping $(serverip)
> > Using Blackfin EMAC device
> > host 64.233.161.147 is alive
>
> Note that the use of "$(...)" is deprecated. Please use "${...}"
> instead,
OK.
> > Signed-off-by: Robin Getz <rg...@blackfin.uclinux.org>
> > Signed-off-by: Pieter Voorthuijsen <pieter.voorthuij...@prodrive.nl>
> >
> >
> > common/cmd_net.c | 32 ++++
> > include/configs/bfin_adi_common.h | 7
> > include/net.h | 4
> > net/Makefile | 1
> > net/dns.c | 213 ++++++++++++++++++++++++++++
> > net/dns.h | 38 ++++
> > net/net.c | 20 ++
> > 7 files changed, 314 insertions(+), 1 deletion(-)
>
> 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 ?
> > Index: include/net.h
> > ===================================================================
> > --- include/net.h (revision 1968)
> > +++ include/net.h (working copy)
>
> Could you generate a git patch instead?
Sure - I'll generate something from the net tree?
> > @@ -361,6 +361,10 @@
> > /* from net/net.c */
> > extern char BootFile[128]; /* Boot File name
> > */
> >
> > +#if defined(CONFIG_CMD_DNS)
> > +extern char NetDNSResolve[255]; /* The host to resolve
> > */
> > +#endif
>
> Can this buffer overflow?
Shouldn't.
+ if (strlen(argv[1]) > sizeof(NetDNSResolve) - 1) {
+ puts("Name too long.\n");
+ return -1;
+ }
+
> > Index: net/dns.c
> > ===================================================================
> > --- net/dns.c (revision 0)
> > +++ net/dns.c (revision 0)
> > @@ -0,0 +1,213 @@
> > +/*
> > + * DNS support driver
> > + *
> > + * Copyright (c) 2008 Pieter Voorthuijsen <pieter.voorthuij...@prodrive.nl>
> > + * Copyright (c) 2009 Robin Getz <rg...@blackfin.uclinux.org>
> > + *
> > + * This is a simple DNS implementation for U-Boot. It will use the first IP
> > + * in the DNS response as NetServerIP. This can then be used for any other
> > + * network related activities.
>
> 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.
Originally I thought about dnsip, but that pre-existing, and is used to store
the nameserver ip (which is poorly named IMHO).
any better suggestions welcome.
> > +char NetDNSResolve[255]; /* The host to resolve */
>
> See above.
See answer :)
> > +static int DnsOurPort;
> > +
> > +static void
> > +DnsSend(void)
> > +{
> ...
> > + do {
> > + s = strchr(name, '.');
> > + if (!s)
> > + s = name + name_len;
> > +
> > + n = s - name; /* Chunk length */
> > + *p++ = n; /* Copy length */
> > + memcpy(p, name, n); /* Copy chunk */
> > + p += n;
> > +
> > + if (*s == '.')
> > + n++;
> > +
> > + name += n;
> > + name_len -= n;
> > + } while (*s != '\0');
> > +
> > + *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.
> > +static void
> > +DnsHandler(uchar *pkt, unsigned dest, unsigned src, unsigned len)
> > +{
> ...
> > + /* Received 0 answers */
> > + if (header->nanswers == 0) {
> > + debug("DNS server returned no answers\n");
> > + puts("server can't find hostname\n");
>
> Debug and regular output are redundant. Omit the debug().
> Actually I recommend to use the debug() message text, which is IMO
> more precise.
No problem.
> > + if (&p[5] > e || ntohs(tmp) != DNS_A_RECORD) {
> > + debug("Query was not A record\n");
> > + puts("server can't find IP number of hostname\n");
>
> Ditto.
>
> > + if (p + dlen <= e) {
> > + ip_to_string(IPAddress, IPStr);
> > + NetServerIP = IPAddress;
> > + setenv("serverip", IPStr);
>
> I object to this part. I think it is a very bad idea to meddle with
> the NetServerIP and "serverip" settings here - this may be wanted by
> the user, or maybe not.
>
> I am aware that we don't have an easy way of passing results from a
> command back to U-Boot's "shell", so I suggest a syntactical change,
> see below.
OK.
> > Index: net/Makefile
> > ===================================================================
> > --- net/Makefile (revision 1968)
> > +++ net/Makefile (working copy)
> > @@ -34,6 +34,7 @@
> > COBJS-y += eth.o
> > COBJS-y += nfs.o
> > COBJS-$(CONFIG_CMD_SNTP) += sntp.o
> > +COBJS-$(CONFIG_CMD_DNS) += dns.o
>
> Please keep list sorted.
OK
> > Index: common/cmd_net.c
> > ===================================================================
> > --- common/cmd_net.c (revision 1968)
> > +++ common/cmd_net.c (working copy)
> ...
> > +U_BOOT_CMD(
> > + dns, 2, 1, do_dns,
> > + "lookup the IP of a hostname",
> > + "[machine to lookup]\n"
> > +);
>
> I suggest to change "machine to lookup" into "hostname".
OK
> Hmmm... is this argument really optional as the brackets suggest? I
> don't think so. And why do you allow for 2 arguments? And the newline
> is bogus, too.
Yeah, sorry - I don't completely grok the U-Boot help syntax. I'll fix.
> I suggest to change this as follows:
>
> U_BOOT_CMD(
> dns, 2, 1, do_dns,
> "lookup the IP of a hostname",
> "hostname [envvar]"
> );
>
> If you use the command with one argument (just the host name to look
> up), it will only print the result, like this:
>
> => dns www.denx.de
> 85.214.87.163
>
> Note that this command does NOT change any environment settings!
>
> To do the equivalent of your implementation, you have to tell the
> command the name of the environment variable where trhe result (if
> any) shall be stored:
>
> => dns www.denx.de serverip
> 85.214.87.163
>
> In this case the command will print the result _and_ store the value
> in the environment variable given on the command line. This seems more
> flexible to me, as I can then also do things like
>
> => dns ${hostname} ipaddr
>
> etc.
>
> What do you think?
that is more flexible that the current implementation...
but also a little more complex.
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...
-Robin
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
