[Swan-dev] questions about find_next_v2_host_connection

[D. Hugh Redelmeier]

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.
 */
if (c->config->ike_version != ike_version)
	continue;
if ((req_policy ^ c->policy) & policy_exact_mask)
	continue;

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 */
}

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).

(2) seems irrelevant to v2 and hence this function.

The third test in the code seems unrelated to the comment (but the
visual structure suggests otherwise).

================

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 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;
}

================

Because the code was cloned, similar changes should be made to
ikev1_host_pair.c

================

Does anyone object to my making these changes?