[Swan-dev] questions about find_next_v2_host_connection

[Andrew Cagney]

On Sun, 19 Dec 2021 at 17:54, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
>
> Why does this deal with V1?
>
> Its name includes "v2", so that suggests that it does not.
>
> It has a local variable "ike_version" which is immutably IKEv2.
>
> And yet it has a comment that includes the line
>
>         * (2) kind of IKEV1 (POLICY_AGGRESSIVE)
>
> ================
>
> /*
>  * Success may require exact match of:
>  * (1) XAUTH (POLICY_XAUTH)
>  * (2) kind of IKEV1 (POLICY_AGGRESSIVE)
>  * (3) IKE_VERSION
>  * So if any bits are on in the exclusive OR, we fail.
>  * Each of our callers knows what is known so specifies
>  * the policy_exact_mask.
>  */

Policy bits just aren't what they used to be.

The references to specific bits are all showing their age (at one
point (3) was POLICY bits) and should be deleted, however the general
comment about how the below works still holds true.

> if ((req_policy ^ c->policy) & policy_exact_mask)
>         continue;

However, for IKEv2, is policy_exact_match ever !=LEMPTY?

> The comment has numbered terms, but the code does not reflect this
> numbering (as far as I can tell).  For example, the first test is for
> (3).

> ================
>
> if (peer_id != NULL && !same_id(peer_id, &c->spd.that.id) &&
>     (c->spd.that.id.kind != ID_FROMCERT && !id_is_any(&c->spd.that.id))) {
>                 continue; /* incompatible ID */
> }

I don't find this hard to read:
- peer ID != NULL && *peer_id != c->spd.that.id)
  i.e., the peer id valid and doesn't exact match, not good so far
and
- (c->spd.that.id.kind != ID_FROMCERT && !id_is_any(&c->spd.that.id)
there's no reason it shouldn't

toss this one and CONTINUE to the next choice

> This certainly deserves a comment.  It seems to come from
> b920bc4cba68b9234e7e15e03d44837587ee682d but may have been copied from
> elsewhere.
>
> I don't immediately understand this test.
>
> The structure suggests that the && at the end of the first line might
> have been intended to be a ||.  I'll ignore this.
>
> && is associative.  So the parens opening at the start of the second
> line are redundant.
>
> Applying de Morgan's law would make this easier to understand.  This
> replaces four negations with one.
>
> if (!(peer_id == NULL || same_id(peer_id, &c->spd.that.id) ||
>       c->spd.that.id.kind == ID_FROMCERT || id_is_any(&c->spd.that.id))) {
>                 continue; /* incompatible ID */
> }

> This might be clearer with each disjunct on a separate line:
>
> if (!(peer_id == NULL ||
>       same_id(peer_id, &c->spd.that.id) ||
>       c->spd.that.id.kind == ID_FROMCERT ||
>       id_is_any(&c->spd.that.id)
>     )) {
>                 continue; /* incompatible ID */
> }
>
> Even better: absorb the rest of the loop inside the IF, getting rid of the
> last negation:
>
> if (peer_id == NULL ||
>     same_id(peer_id, &c->spd.that.id) ||
>     c->spd.that.id.kind == ID_FROMCERT ||
>     id_is_any(&c->spd.that.id)
> {
>         /*
>          * Success if all specified policy bits are in candidate's policy.
>          * It works even when the exact-match bits are included.
>          */
>         if ((req_policy & ~c->policy) == LEMPTY)
>                 break;
> }

The code block largely follows this structure:

  for each thing
       if thing fails a test
            continue
       if thing fails a test
            continue
       if thing fails a test
            continueif thing fails a test
        winner, break, or look for better
it would be good to keep it

>
> Does anyone object to my making these changes?
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev