[lwip-devel] [task #13106] Add IPv6 scopes

[David van Moolenbroek]

Follow-up Comment #23, task #13106 (project lwip):

Dirk, I'm afraid your fix for Axel Lin's reported issue is not the full fix,
because this wasn't just a compilation warning - I actually messed it up in
such a way that the resulting behavior was wrong. The attached patch 0001,
against master, resolves the bug.

Simon:

> icmp6 looks a bit strange, could we change icmp6_send_response to not rely
on "NULL" but instead create sub-functions handling NULL or the old style? 

Something like attached patch 0002..? I'm hesitant to make two completely
separate versions of the send-response code, because there is a bit too much
shared code between the two..

> the diff of ip6_route() confused me. Due to lack of time, for now I hope
without scope support, the behaviour doesn't change. I'll try to come back on
that when I find the time...

That is indeed the intention. The old algorithm was "if dest is link-local,
find the interface that owns src" and that is what the new algorithm should
reduce to without scopes support, too.

There are two new additions even that case though. The first is a test for
multicast destinations, which was previously an exception in the UDP code.
With scopes support disabled, that is not really an improvement but not really
a bad thing either. The second is the rule "if src is link-local, even if dest
is not, also find the interface that owns src". From what I've seen, the
previous code largely ignored the (rare but real) possibility that a
link-local source address would be combined with a global (or ULA etc)
destination address, so that addition should be a small improvement.

> having 'packed' in the copy_from/to_packed macros seems a bit strange. I
know where the 'packed' comes from (because the packed typedefs are used in
protocol structs), but we might need a better (self-explaining) name here
(although I can't come up with one right now ;) 

Hmm, I guess the most logical alternative (even if strictly not necessarily
always correct) would be some suffix that indicates the address is in a packet
header, so perhaps _from/_to_header or _from/_to_packet or so? If we can
settle on a name I'd be happy to make a patch. Either way this should be done
ASAP though, in particular before the lwip-contrib code is repaired..

> tcp_connect: when scopes are disabled, calling ip_route is of no use

In hindsight I should have made a separate patch out of that entire change. My
considerations were the following, though:

1) my addition is in the else-clause of an if/else block that tests whether
the PCB was bound to an address; the else-clause is for the case that the TCP
PCB is indeed bound before connecting, which should be relatively rare in
practice and so this should not be the common path;
2) the entire if/else block is directly preceded by a comment saying "check if
we have a route to the remote host", suggesting that the route lookup in the
if-clause is not done strictly for the purpose of picking a source address;
3) in fact, *not* having the route lookup in the else-clause creates
behavioral disparity between the two cases: since the return value of
tcp_output() is not checked later, the behavior of tcp_connect() with an
unreachable destination would depend on whether the PCB was bound or not: if
not bound, it would fail immediately, while if bound, it would eventually time
out;
4) with the previous 0002-tcp-eliminate.. patch, even if IPv6 or scopes
support is disabled, the route lookup is useless only if
TCP_CALCULATE_EFF_SEND_MSS is also disabled, even though that setting is
enabled in opt.h by default.

In other words, this extra route lookup will only be useless for people who a)
have IPv6 or scopes support disabled, b) have effective-send-MSS support
explicitly disabled, c) bind their TCP sockets before connecting them, *and*
d) do not care about how tcp_connect() behaves in the light of unreachable
destinations. For everyone else, the new situation should be no worse and
possibly better than before.

It is of course possible to do lazy lookups instead, but it will not make the
code prettier. What do you think?

> There's no IPADDR6_INIT() taking a zone, would we need this?

Doubtful, because that is a static initializer while an implementation should
not rely on netif->num having a specific value at compile time..

> Oh, and it seemed like the CUSTOM define was used in '#if' although
undefined...

That should not be the case: if scopes support is enabled, it is defined to 0
if not previously defined; if scopes support is disabled, it is always defined
to 0.

(file #39660, file #39661)
    _______________________________________________________

Additional Item Attachment:

File name: 0001-ip6-fix-parentheses-in-ip6_addr_has_scope.patch Size:1 KB
File name: 0002-ip6-split-icmp6_send_response-from-_with_addrs.patch Size:3 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?13106>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/