This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
Re: RedBoot Network support fix
- From: Gary Thomas <gary at mlbassoc dot com>
- To: Yoshinori Sato <ysato at users dot sourceforge dot jp>
- Cc: eCos patches <ecos-patches at ecos dot sourceware dot org>
- Date: Mon, 07 Feb 2005 05:46:17 -0700
- Subject: Re: RedBoot Network support fix
- Organization: MLB Associates
- References: <m24qh3wkjs.wl%ysato@users.sourceforge.jp> <1107279815.18185.110.camel@hermes> <m2651ai6hz.wl%ysato@users.sourceforge.jp> <1107427767.18185.247.camel@hermes> <m2u0oolvav.wl%ysato@users.sourceforge.jp>
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.
--
------------------------------------------------------------
Gary Thomas | Consulting for the
MLB Associates | Embedded world
------------------------------------------------------------