[lwip-users] Re: [lwip] lwip-cvs-20020108 errors

[Adam Dunkels]

Hi Horst!

On Tuesday 08 January 2002 16.12, you wrote:
> ----------------------------------------------------------------
> ip.c
> in ip_input() function:
>
> netmask-independant ip broadcast (255.255.255.255) is not recognized
> correct!
>
>      if(ip_addr_isany(&(netif->ip_addr)) ||
>         ip_addr_cmp(&(iphdr->dest), &(netif->ip_addr)) ||
>         (ip_addr_isbroadcast(&(iphdr->dest), &(netif->netmask)) &&
>          ip_addr_maskcmp(&(iphdr->dest), &(netif->ip_addr),
> &(netif->netmask)))) {
>        break;
>      }
>
> Your function ip_addr_isbroadcast() works correct but it is ANDed with
> ip_addr_maskcmp() - why? Only local broadcasts are recognized in that way.
> General Broadcasts (255.255.255.255) are dropped!
> Without ip_addr_maskcmp() general broadcasts are recognized too.

Just removing the ip_addr_maskcmp() would introduce problems since this would 
mean that broadcasts to other networks would be let through. A packet to 
192.168.255.255 would pass even if "our" network was 10.0.255.255, which we 
don't want. Instead an additional check should be added: 
ip_addr_cmp(&(iphdr->dest), IP_ADDR_BROADCAST). This would explicitly check 
for packets to 255.255.255.255.

> -----------------------------------------------------------------
> inet.h
>
> please add lower case define's (needed for big endian):
>
> #ifndef NTOHS
> #   define NTOHS HTONS
> #   define ntohs htons  /*+hg added*/
> #endif /* NTOHS */
>
> #ifndef NTOHL
> #   define NTOHL HTONL
> #   define ntohl htonl  /*+hg added*/
> #endif /* NTOHL */
>
> because there are serveral "ntohl()" -lower case- calls
> in your sources.

Ok! I did it a bit differently that you proposed to avoid collisions between 
macro names and function names. I removed the definition of the ntoh* 
functions and added some #undefs in case the #defines already had been 
defined by some other header file.

> ------------------------------------------------------------------
> core/udp.c
>
> udp_input() function
>
> There is a problem in checksum calculation with your last changes:
>
>    iphdr = p->payload;
>    udphdr = (struct udp_hdr *)((u8_t *)p->payload + IPH_HL(iphdr) *
> 4/sizeof(u8_t));
>
>    pbuf_header(p, -(UDP_HLEN + IPH_HL(iphdr) * 4));
>
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> p is adjusted to point _behind_ udp-header! This doesn't work with the
> later checksum calculation inet_chksum_pseudo()!
>
> You should change the adjust before checksum calculation to
>
>   pbuf_header(p, -(IPH_HL(iphdr) * 4)); /*+hg*/
>
> and after checksum calculation with another
>
>   pbuf_header(p, -(UDP_HLEN));  /*+hg*/
>
> to the right data pointer.

Thanks! The reason is that I moved the checksum calculation so that the 
checksum only is calculated for datagrams that we really are interested in 
(to avoid doing a lot of checksum calculations for broadcast UDP traffic such 
as the Microsoft SMB filesystem).

Thanks a lot, as always! 

/adam
-- 
Adam Dunkels <address@hidden>
http://www.sics.se/~adam

[This message was sent through the lwip discussion list.]