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,
> 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.
> Index: include/net.h
> ===================================================================
> --- include/net.h (revision 1968)
> +++ include/net.h (working copy)
Could you generate a git patch instead?
> @@ -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?
> 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.
> +char NetDNSResolve[255]; /* The host to resolve */
See above.
> +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?
> +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.
> + 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.
> 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.
> 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".
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.
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?
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
Killing is wrong.
-- Losira, "That Which Survives", stardate unknown
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
