This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
Re: RedBoot Network support fix
On Thu, 2005-02-10 at 23:31 +0900, Yoshinori Sato wrote:
> At Mon, 07 Feb 2005 05:46:17 -0700,
> Gary Thomas wrote:
> >
> > On Mon, 2005-02-07 at 15:48 +0900, Yoshinori Sato wrote:
> > > At Thu, 03 Feb 2005 03:49:27 -0700,
> > > Gary Thomas wrote:
> > > >
> > > > On Thu, 2005-02-03 at 13:59 +0900, Yoshinori Sato wrote:
> > > > > At Tue, 01 Feb 2005 10:43:35 -0700,
> > > > > Gary Thomas wrote:
> > > > > >
> > > > > > On Thu, 2005-01-27 at 13:41 +0900, Yoshinori Sato wrote:
> > > > > > > I fix the handling of IP address
> > > > > >
> > > > > > I have some questions about these changes.
> > > > > >
> > > > > > >
> > > > > > > * src/main.c (cyg_start): Fix compiler warning with
> > > > > > > Index: packages/redboot/current/src/net/bootp.c
> > > > > > > ===================================================================
> > > > > > > RCS file: /cvsroot/ecos-h8/ecos/packages/redboot/current/src/net/bootp.c,v
> > > > > > > retrieving revision 1.1.1.6
> > > > > > > retrieving revision 1.2
> > > > > > > diff -u -r1.1.1.6 -r1.2
> > > > > > > --- packages/redboot/current/src/net/bootp.c 4 Mar 2004 05:23:06 -0000 1.1.1.6
> > > > > > > +++ packages/redboot/current/src/net/bootp.c 27 Jan 2005 04:23:46 -0000 1.2
> > > > > > > @@ -135,6 +135,7 @@
> > > > > > > // The request message has been sent, only accept an ack reply
> > > > > > > if (*p == DHCP_MESS_TYPE_ACK) {
> > > > > > > dhcpState = DHCP_ACK;
> > > > > > > + bp_info->bp_siaddr = b->bp_siaddr;
> > > > > > > return;
> > > > > > > } else {
> > > > > > > expected = DHCP_MESS_TYPE_ACK;
> > > > > >
> > > > > > What is this supposed to do? The default server is already being
> > > > > > obtained from the DHCP response (at least it works properly on all
> > > > > > the targets I've tested).
> > > > >
> > > > > What I put by testing stayed.
> > > > > This is useless.
> > > > >
> > > > > > > Index: packages/redboot/current/src/net/http_client.c
> > > > > > > ===================================================================
> > > > > > > RCS file: /cvsroot/ecos-h8/ecos/packages/redboot/current/src/net/http_client.c,v
> > > > > > > retrieving revision 1.1.1.7
> > > > > > > retrieving revision 1.7
> > > > > > > diff -u -r1.1.1.7 -r1.7
> > > > > > > --- packages/redboot/current/src/net/http_client.c 12 Jan 2005 02:42:44 -0000 1.1.1.7
> > > > > > > +++ packages/redboot/current/src/net/http_client.c 27 Jan 2005 04:20:51 -0000 1.7
> > > > > > > @@ -86,7 +86,12 @@
> > > > > > > struct _stream *s = &http_stream;
> > > > > > >
> > > > > > > if (!info->server->sin_port)
> > > > > > > - info->server->sin_port = 80; // HTTP port
> > > > > > > + info->server->sin_port = htons(80); // HTTP port
> > > > > > > + if (info->server->sin_addr.s_addr == 0) {
> > > > > > > + // server address 0.0.0.0
> > > > > > > + *err = HTTP_OPEN;
> > > > > > > + return -1;
> > > > > > > + }
> > > > > > > if ((res = __tcp_open(&s->sock, info->server, get_port++, 5000, err)) < 0) {
> > > > > > > *err = HTTP_OPEN;
> > > > > > > return -1;
> > > > > >
> > > > > > This one seems OK.
> > > > > >
> > > > > > > Index: packages/redboot/current/src/net/net_io.c
> > > > > > > ===================================================================
> > > > > > > RCS file: /cvsroot/ecos-h8/ecos/packages/redboot/current/src/net/net_io.c,v
> > > > > > > retrieving revision 1.1.1.8
> > > > > > > retrieving revision 1.9
> > > > > > > diff -u -r1.1.1.8 -r1.9
> > > > > > > --- packages/redboot/current/src/net/net_io.c 23 Nov 2004 09:00:42 -0000 1.1.1.8
> > > > > > > +++ packages/redboot/current/src/net/net_io.c 27 Jan 2005 04:22:29 -0000 1.9
> > > > > > > @@ -678,6 +678,7 @@
> > > > > > > #endif
> > > > > > > #ifdef CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR
> > > > > > > char ip_addr[16];
> > > > > > > + struct in_addr default_addr;
> > > > > > > #endif
> > > > > > >
> > > > > > > // Set defaults as appropriate
> > > > > > > @@ -782,7 +783,9 @@
> > > > > > > #ifdef CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR
> > > > > > > diag_sprintf(ip_addr, "%d.%d.%d.%d",
> > > > > > > CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR);
> > > > > > > - inet_aton(ip_addr, &my_bootp_info.bp_siaddr);
> > > > > > > + inet_aton(ip_addr, &default_addr);
> > > > > > > + if (default_addr.s_addr)
> > > > > > > + my_bootp_info.bp_siaddr = default_addr;
> > > > > > > #endif
> > > > > > > #ifdef CYGSEM_REDBOOT_FLASH_CONFIG
> > > > > > > flash_get_IP("bootp_server_ip", (ip_addr_t *)&my_bootp_info.bp_siaddr);
> > > > > >
> > > > > > This seems pointless. If the server IP address has been configured in,
> > > > > > it should be set to a legal, non-zero, value. All you've done is
> > > > > > duplicate the existing behaviour that if the default server is not set
> > > > > > (i.e. is 0.0.0.0) by fconfig, then get it from DHCP.
> > > > > >
> > > > > > Please explain.
> > > > >
> > > > > Because CYGDAT_REDBOOT_BOOTP_SERVER_IP_ADDR is defined by "0,0,0,0" when do not use fconfig,
> > > > > default server which got with dhcp is destroyed.
> > > >
> > > > My point is that CYGDAT_REDBOOT_BOOTP_SERVER_IP_ADDR should only be
> > > > used to define a valid server IP address.
> > >
> > > I ignored it in case of "0, 0, 0, 0".
> > >
> > > Index: packages/redboot/current/src/net/net_io.c
> > > ===================================================================
> > > RCS file: /cvsroot/ecos-h8/ecos/packages/redboot/current/src/net/net_io.c,v
> > > retrieving revision 1.1.1.8
> > > diff -u -r1.1.1.8 net_io.c
> > > --- packages/redboot/current/src/net/net_io.c 23 Nov 2004 09:00:42 -0000 1.1.1.8
> > > +++ packages/redboot/current/src/net/net_io.c 4 Feb 2005 15:31:00 -0000
> > > @@ -174,6 +174,9 @@
> > > #define TELNET_DO 0xFD // Will you XXX
> > > #define TELNET_TM 0x06 // Time marker (special DO/WONT after IP)
> > >
> > > +#define IP_ADDR(ADDR) _IP_ADDR(ADDR)
> > > +#define _IP_ADDR(a, b, c, d) ((a << 24) | (b << 16) | (c << 8) | d)
> > > +
> > > static cyg_bool
> > > _net_io_getc_nonblock(void* __ch_data, cyg_uint8* ch)
> > > {
> > > @@ -676,7 +679,8 @@
> > > char *default_devname;
> > > int default_index;
> > > #endif
> > > -#ifdef CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR
> > > +#if defined(CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR) && \
> > > + IP_ADDR(CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR) != 0
> > > char ip_addr[16];
> > > #endif
> > >
> > > @@ -779,7 +783,8 @@
> > > }
> > > if (have_net) {
> > > show_eth_info();
> > > -#ifdef CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR
> > > +#if defined(CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR) && \
> > > + IP_ADDR(CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR) != 0
> > > diag_sprintf(ip_addr, "%d.%d.%d.%d",
> > > CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR);
> > > inet_aton(ip_addr, &my_bootp_info.bp_siaddr);
> > >
> >
> > This still seems the wrong way to me. Either the configuration
> > defines a valid server IP via CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR
> > or that should be undefined and let the DHCP/BOOTP or FCONFIG define it.
> >
>
> I tried to set for action of description mentioned in redboot.cdl.
>
> Index: packages/redboot/current/src/net/net_io.c
> ===================================================================
> RCS file: /cvsroot/ecos-h8/ecos/packages/redboot/current/src/net/net_io.c,v
> retrieving revision 1.1.1.8
> diff -u -r1.1.1.8 net_io.c
> --- packages/redboot/current/src/net/net_io.c 23 Nov 2004 09:00:42 -0000 1.1.1.8
> +++ packages/redboot/current/src/net/net_io.c 10 Feb 2005 14:20:39 -0000
> @@ -672,6 +672,7 @@
> cyg_netdevtab_entry_t *t;
> unsigned index;
> struct eth_drv_sc *primary_net = (struct eth_drv_sc *)0;
> + bool bootp_success = false;
> #if defined(CYGHWR_NET_DRIVERS) && (CYGHWR_NET_DRIVERS > 1)
> char *default_devname;
> int default_index;
> @@ -758,6 +759,7 @@
> if (use_bootp) {
> if (__bootp_find_local_ip(&my_bootp_info) == 0) {
> have_net = true;
> + bootp_success = true;
> } else {
> // Is it an unset address, or has it been set to a static addr
> if (__local_ip_addr[0] == 0 && __local_ip_addr[1] == 0 &&
> @@ -780,9 +782,12 @@
> if (have_net) {
> show_eth_info();
> #ifdef CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR
> - diag_sprintf(ip_addr, "%d.%d.%d.%d",
> - CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR);
> - inet_aton(ip_addr, &my_bootp_info.bp_siaddr);
> + if (!bootp_success) {
> + // Because failed in BOOTP, use a specification address.
> + diag_sprintf(ip_addr, "%d.%d.%d.%d",
> + CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR);
> + inet_aton(ip_addr, &my_bootp_info.bp_siaddr);
> + }
> #endif
> #ifdef CYGSEM_REDBOOT_FLASH_CONFIG
> flash_get_IP("bootp_server_ip", (ip_addr_t *)&my_bootp_info.bp_siaddr);
Sorry, I still disagree with this approach. If the user has
configured CYGDAT_REDBOOT_DEFAULT_BOOTP_SERVER_IP_ADDR then
that should be what is used, no matter what. If you want to
rely on the BOOTP answer, then leave this undefined. Note
that this is exactly the way the 'fconfig' variable for the
default server is handled and I think that it should be consistent
and is logical the way it is.
I was happy with your other changes - can you send a patch for just
those, please?
--
------------------------------------------------------------
Gary Thomas | Consulting for the
MLB Associates | Embedded world
------------------------------------------------------------