[U-Boot] [PATCH] - add dns

[Robin Getz]

On Fri 17 Jul 2009 16:55, Wolfgang Denk pondered:
> Dear Robin Getz,
> 
> In message <200907171553.08108.rgetz at 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 at blackfin.uclinux.org>
> > Signed-off-by: Pieter Voorthuijsen <pieter.voorthuijsen at 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 at prodrive.nl>
> > + * Copyright (c) 2009 Robin Getz <rgetz at 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