Re: [ECOS] build_bootp_record() support dns server address

[Andrew Lunn <andrew dot lunn at ascom dot ch>]

Hi Motoya

There are some things i don't like about this. 

Firstly, the DNS address is not a properly of the interface. With
multiple interfaces you do not expect multiple DNS servers. Quite
likely, the DHCP servers on the different interfaces will return the
same DNS server. Or maybe only one DHCP server will reply with a DNS
server and that server maybe routed to via the other interface etc....

Even if there are two DNS servers returned by DHCP, the current DNS
client only allows for one DNS server. So, its a case of last one
wins.

Secondly, build_bootp_record() is a public API function. I have my IP
addresses, network masks etc, stored in flash. I use this function to
build a bootp record which i then pass to init_net(). Your change will
break my and others code.

I have a counter proposal. 

Two new CDL options. A bool CYGPKG_NS_DNS_DEFAULT which
enables/disables the use of a default address. A data
CYGDAT_NS_DNS_DEFAULT_SERVER which is the IP address of the server. 

init_all_network_interfaces() then has some code inside

#ifdef CYGPKG_NS_DNS_DEFAULT_SERVER

to turn the string into an struct in_addr and pass it to
cyg_dns_res_init()

        Andrew


On Thu, Jan 09, 2003 at 06:56:47PM +0900, Motoya Kurotsu wrote:
> Hi, jifl;
> 
> I would like to submit the patch. Please check it.
> 
> Motoya Kurotsu
> Allied Telesis K.K.
> 
> ===============================================================
> 
> diff -uNr common.orig/current/ChangeLog common/current/ChangeLog
> --- common.orig/current/ChangeLog	Sun Jan  5 03:46:39 2003
> +++ common/current/ChangeLog	Thu Jan  9 18:44:17 2003
> @@ -1,3 +1,9 @@
> +2003-01-09  Motoya Kurotsu  <kurotsu@allied-telesis.co.jp>
> +
> +	* src/network_support.c (build_bootp_record): 
> +	* include/bootp.h: 
> +	* cdl/net.cdl: Add statical dns server address support
> +
>  2003-01-04  Jonathan Larmour  <jifl@eCosCentric.com>
>  
>  	* doc/tcpip.sgml: Use new entity name for tcpip manpages.
> diff -uNr common.orig/current/cdl/net.cdl common/current/cdl/net.cdl
> --- common.orig/current/cdl/net.cdl	Tue May 21 07:25:03 2002
> +++ common/current/cdl/net.cdl	Thu Jan  9 18:27:41 2003
> @@ -509,6 +509,12 @@
>                  flavor  data
>                  default_value { "192.168.1.101" }
>              }
> +
> +            cdl_option CYGHWR_NET_DRIVER_ETH0_ADDRS_DNS_SERVER {
> +                display "DNS server IP address for 'eth0'"
> +                flavor  data
> +                default_value { "192.168.1.102" }
> +            }
>          }
>      }
>  
> @@ -637,6 +643,12 @@
>                  display "Server IP address for 'eth1'"
>                  flavor  data
>                  default_value { "192.168.1.101" }
> +            }
> +
> +            cdl_option CYGHWR_NET_DRIVER_ETH1_ADDRS_DNS_SERVER {
> +                display "DNS server IP address for 'eth1'"
> +                flavor  data
> +                default_value { "192.168.1.102" }
>              }
>          }
>      }
> diff -uNr common.orig/current/include/bootp.h common/current/include/bootp.h
> --- common.orig/current/include/bootp.h	Tue May 21 07:25:03 2002
> +++ common/current/include/bootp.h	Thu Jan  9 18:19:32 2003
> @@ -325,7 +325,8 @@
>                     const char *addrs_netmask,
>                     const char *addrs_broadcast,
>                     const char *addrs_gateway,
> -                   const char *addrs_server);
> +                   const char *addrs_server,
> +                   const char *addrs_dns_server);
>  
>  // Do bootp to fill in the bootp record from the net (other interfaces must
>  // be down for this to work, because of the "half-up" state of the
> diff -uNr common.orig/current/src/network_support.c common/current/src/network_support.c
> --- common.orig/current/src/network_support.c	Tue May 21 07:25:05 2002
> +++ common/current/src/network_support.c	Thu Jan  9 18:17:52 2003
> @@ -201,7 +201,8 @@
>                     const char *addrs_netmask,
>                     const char *addrs_broadcast,
>                     const char *addrs_gateway,
> -                   const char *addrs_server)
> +                   const char *addrs_server,
> +                   const char *addrs_dns_server)
>  {
>      int i, s;
>      in_addr_t addr;
> @@ -241,6 +242,8 @@
>      vp = add_tag(vp, TAG_IP_BROADCAST, &addr, sizeof(in_addr_t));
>      addr = inet_addr(addrs_gateway);
>      vp = add_tag(vp, TAG_GATEWAY, &addr, sizeof(in_addr_t));
> +    addr = inet_addr(addrs_dns_server);
> +    vp = add_tag(vp, TAG_DOMAIN_SERVER, &addr, sizeof(in_addr_t));
>      *vp = TAG_END;
>  }
>  
> @@ -312,7 +315,8 @@
>                             string(CYGHWR_NET_DRIVER_ETH0_ADDRS_NETMASK),
>                             string(CYGHWR_NET_DRIVER_ETH0_ADDRS_BROADCAST),
>                             string(CYGHWR_NET_DRIVER_ETH0_ADDRS_GATEWAY),
> -                           string(CYGHWR_NET_DRIVER_ETH0_ADDRS_SERVER));
> +                           string(CYGHWR_NET_DRIVER_ETH0_ADDRS_SERVER),
> +                           string(CYGHWR_NET_DRIVER_ETH0_ADDRS_DNS_SERVER));
>          show_bootp(eth0_name, &eth0_bootp_data);
>  #endif
>  #ifdef CYGPKG_IO_PCMCIA
> @@ -366,7 +370,8 @@
>                             string(CYGHWR_NET_DRIVER_ETH1_ADDRS_NETMASK),
>                             string(CYGHWR_NET_DRIVER_ETH1_ADDRS_BROADCAST),
>                             string(CYGHWR_NET_DRIVER_ETH1_ADDRS_GATEWAY),
> -                           string(CYGHWR_NET_DRIVER_ETH1_ADDRS_SERVER));
> +                           string(CYGHWR_NET_DRIVER_ETH1_ADDRS_SERVER),
> +                           string(CYGHWR_NET_DRIVER_ETH1_ADDRS_DNS_SERVER));
>          show_bootp(eth1_name, &eth1_bootp_data);
>  #endif
>  #ifdef CYGPKG_IO_PCMCIA
> 
> 
> 
> 
> On Thu, Jan 09, 2003 at 06:54:59AM +0000, Jonathan Larmour wrote:
> > Motoya Kurotsu wrote:
> > > Hi, all;
> > > 
> > > I would like to hear your advice about my idea regarding 
> > > init_all_network_interfaces().
> > > 
> > > When ip address is staticaly set (i.e. CYGHWR_NET_DRIVER_ETHX_BOOTP is 
> > > undefined), init_net() doesn't call cyg_dns_res_init(). So I think it is 
> > > better to modify build_bootp_record() to have the dns server address 
> > > like in the following:
> > > 
> > >     build_bootp_record(struct bootp *bp,
> > > 		       const char *if_name,
> > > 		       const char *addrs_ip,
> > > 		       const char *addrs_netmask,
> > > 		       const char *addrs_broadcast,
> > > 		       const char *addrs_gateway,
> > > 		       const char *addrs_server,
> > > 		       const char *addrs_dns_server);	<---
> > > 
> > > Also the dns server address might be added in cdl script like other addresses.
> > > The attachment is my patch for network_support.c for reference.
> > 
> > Looks pretty reasonable to me. All that remains is to add the CDL option 
> > for CYGHWR_NET_DRIVER_ETH1_ADDRS_DNS_SERVER, write a ChangeLog entry and 
> > submit it to <ecos-patches@sources.redhat.com> and I'll commit it for you.
> >