Re: [U-Boot] [PATCH] - add dns

17 Jul 2009

On Fri 17 Jul 2009 16:55, Wolfgang Denk pondered:
...
Dear Robin Getz,
In message 200907171553.08108.rgetz@blackfin.uclinux.org you wrote:
...
On 04 Oct 2008 Pieter posted a dns implementation for U-Boot.
http://www.mail-archive.com/u-boot-users@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 rgetz@blackfin.uclinux.org
Signed-off-by: Pieter Voorthuijsen pieter.voorthuijsen@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.voorthuijsen@prodrive.nl



Copyright (c) 2009 Robin Getz rgetz@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

    