[Swan-dev] ikev2 broke in master 500519c..6eca8ba

[D. Hugh Redelmeier]

| From: Tuomo Soini <tis at foobar.fi>

| I upgraded my real world testing packages from 500519c to 6eca8ba and
| noticed ikev2 connections stopped working.

I'm sorry for breaking master!

| I had big difficulty in
| bisecting the actual problem with was:

I guess that means more than one thing is broken.

| bisecting revealed 6d4f4f20d1058cdf36120719691ab128a64e1329 to be first
| bad commit.

I thought that my sequence of commits did not contain any fixes for
previous ones.  So I'm most interested in the first bad one.

I don't immediately see any thing questionable in
6d4f4f20d1058cdf36120719691ab128a64e1329.  I'm pretty sure that I
tested it with a full run of the suite and all was happy.

I'm not saying that 6d4f4f20d1058cdf36120719691ab128a64e1329 is
correct, just that the problem requires deeper digging.

The only (intentional!) serious logic change in that commit is to
build_outgoing_opportunistic_connection.  I eliminated the innermost
loop.

The nest of loops is trying to find the connection with the spd that
best matches the given clients.

Old structure:

    best = NULL
    for each interface
        for each connection "c" with a host pair matching that interface
            for each spd "sr" in that connection
                skip if not routed or doesn't match clients
                if best == NULL
                    best = c
                else
                    for each spd "bestsr" in best
                        if sr is a better match than bestsr
                            best = c
                            break

new structure:

    best = NULL
    bestsr = NULL
    for each interface
        for each connection "c" with a host pair matching that interface
            for each spd "sr" in that connection
                skip if not routed or doesn't match clients
                if best == NULL or sr is a better match than bestsr
                    best = c
                    bestsr = sr

The key difference is that the code remembers bestsr and doesn't need
to do that inner for loop.

I think that this change can only make a difference if a connection
actually has more than one spd.  Does that happen in your environment?

The old code would let c win if any spd of c was better than any spd
of best.  That seems odd (but then again I don't really understand the
spd lists).  I would have expected the one with the best single spd to
win.

It is also true that the test for "better" is a bit odd.

I'd really like to get to the bottom of this.