This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RedBoot Network support fix


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).

> 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.

> Index: packages/redboot/current/src/net/tftp_client.c
> ===================================================================
> RCS file: /cvsroot/ecos-h8/ecos/packages/redboot/current/src/net/tftp_client.c,v
> retrieving revision 1.1.1.9
> retrieving revision 1.2
> diff -u -r1.1.1.9 -r1.2
> --- packages/redboot/current/src/net/tftp_client.c	12 Jan 2005 02:42:44 -0000	1.1.1.9
> +++ packages/redboot/current/src/net/tftp_client.c	27 Jan 2005 04:21:33 -0000	1.2
> @@ -103,6 +103,12 @@
>      tftp_stream.local_addr.sin_addr.s_addr = htonl(INADDR_ANY);
>      tftp_stream.local_addr.sin_port = htons(get_port++);
>  
> +    if (info->server->sin_addr.s_addr == 0) {
> +	// server address 0.0.0.0
> +	*err = TFTP_EBADOP;
> +	return -1;
> +    }
> +
>      if (info->server->sin_port == 0) {
>          info->server->sin_port = htons(TFTP_PORT);
>      } else {
> ============================================================

This one seems OK as well.

Once I understand the rationale behind these changes, I'll apply
them as appropriate to the source tree.

Thanks

-- 
------------------------------------------------------------
Gary Thomas                 |  Consulting for the
MLB Associates              |    Embedded world
------------------------------------------------------------


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]