RE: [lwip-devel] Non-broadcast interfaces (patch)

[Mountifield, Tony]

Hi Leon,

Many thanks for your reply. And thanks to Kieran too, for offering to commit
the patch.

> > Initially I set up a SLIP interface, which I connected to a 
> > SLIP port on a Linux box. The standard subnet mask for a
> > SLIP link is 255.255.255.255, so I specified this in lwip
> > when setting up the interface. With this subnet mask,
> >
> Is this standard SLIP subnet mask specified somewhere? I 
> would expect a 0.0.0.0 network mask on a point-to-point link.

OK, I think I was a little hasty in asserting a "standard". When I set up
the interface on Linux using slattach and ifconfig, I got a subnet mask of
255.255.255.255, and assumed that was the standard. In fact, I think that is
just one option, and other subnet masks could be chosen. This could be
appropriate, for example, when connected to a router/system that will
perform proxy ARP on the SLIP host's behalf, to make it appear to be on the
LAN.

Stevens' TCP/IP Illustrated Vol 1, chapters 3 and 4 describes this. He also
implies that a SLIP link between, say, 192.168.1.1 and 192.168.1.2 should
have a 30-bit subnet mask, 255.255.255.252. I'm not quite convinced, but can
see the argument. In practice (and I've seen this on PPP dialup links), I
don't think there is even a need for the remote IP address to be in the same
"subnet" as the local IP address at all.

I don't think a mask of 0.0.0.0 would be right, since it would match ANY
destination IP address, and wouldn't work when there were multiple
interfaces on different networks.

> > the test macro ip_addr_isbroadcast() always evaluates to 
> > TRUE, which was causing all incoming packets to be discarded.
> >
> TCP packets only, I hope? Broadcast IP traffic should pass at 
> the IP layer.
> (ah yes, your patch explains :)

OK. It was in ICMP (ignore broadcasts), TCP (broadcast doesn't make sense
for TCP) and UDP (don't reply "port unreachable" to broadcasts).

> > I think this test should only
> > be done if the interface is broadcast-capable (e.g. ethernet), e.g.
> >
> > if (((inp->flags & NETIF_FLAG_BROADCAST) &&
> >       ip_addr_isbroadcast(&(iphdr->dest), &(inp->netmask))) ||
> >       ip_addr_ismulticast(&(iphdr->dest))) {
> >
> > Also, slipif_init() should include:
> >
> > netif->flags = NETIF_FLAG_POINTTOPOINT;
> >
> > I have attached a patch file with changes for this to 
> > icmp.c, tcp_in.c, udp.c and slipif.c - please could someone
> > evaluate it and add to CVS?
> Thanks
> 
> Your patch would work. I have looked into the NetBSD code, 
> and it does the same, *plus* more:
> 
> Basically, it says that an address that exactly matches the network
> interface address, is not a broadcast address.
> 
> However, the BSD broadcast check function has an extra 
> argument, which is the interface itself. (BTW, NetBSD's
> in_broadcast() attached below).

Thanks. I notice it makes use of an extra field in netif: the broadcast
address.

> So, for lwIP, we might change the broadcast check function to 
> match BSD's,
> OR
> do the network interface check in the caller's context (every 
> caller will then have two extra conditions built-in).
> 
> I would strongly suggest the former approach, as it hides the 
> two extra checks in the function, making it less error-prone
> and easier maintainable.

I think so too, and change it from a macro to a real function

> BTW, patches are most welcome in the Savannah patches page. 
> It allows us to track patches more easily.

OK, thanks - I'm still learning the ropes.

> Also, are you interested in contributing your port to the 
> "contrib/ports" area of our CVS repository, or is this closed
> contract work?

That decision is not mine to make, but I'm happy to raise it with the
client.

Cheers,
Tony


***********************************************************************************
This email, its content and any attachments is PRIVATE AND
CONFIDENTIAL to TANDBERG Television. If received in error please
notify the sender and destroy the original message and attachments.

www.tandbergtv.com
***********************************************************************************