[Openswan dev] a little nit picking about openswan-2.6.13

[D. Hugh Redelmeier]

In include/packet.h's definition of struct ikev2_trans_attr, the last line 
is a comment:
        /* u_int16_t isatr_value;      Value (AF=0) or not transmitted (AF=1) */

On the assumption that these are Oakley-style attributes, this comment is 
misleading and wrong.

- if a value field like this is present, it is only present if AF=0 so
  the comment on the comment is misleading.

- if a value field is present, it would not be constrained to be an 
  u_int_16_t.  It would be a sequence of bytes with a length indicated
  by the value of isatr_lv.  And the bytes would be in network order (I 
  think), not host order.

- I think I heard that AF=0 never happens in IKEv2 (I don't know).  If so
  the comment is pointless.

================================================================
In include/pluto_constants.h:

#define IS_PARENT_SA(st) ((st)->st_clonedfrom == SOS_NOBODY)
#define IS_CHILD_SA(st)  ((st)->st_clonedfrom != SOS_NOBODY)

Are all states either a parent or a child and never both?

The second definition could be:
  #define IS_CHILD_SA(st)  (!IS_PARENT_SA(st))
This would be more robust because there would be less duplicated code
to diverge.

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

In lib/libopenswan/alg_info.c:

To my eyes, aalg_getbyname_esp's gotos are ugly and unnecessary.

The use of INT_MAX is scary: couldn't we give this designator a
meaningful name (and value) along with all the other values this
variable can take?

Does the function really return an int or is it actually some (perhaps
extended) enum type?

I don't really understand the scanf -- I never use that scary
function.  Should the return value not be tested?  The test after it
looks as if && should be ||, but I don't understand it well enough to
be sure.

modp_getbyname_esp is simpler but its goto is also gratuitous.  This
looks clearer to me:

{
	int ret = -1;

	if (str != NULL && *str != '\0') {
		ret = alg_enum_search_prefix(&oakley_group_names, "OAKLEY_GROUP_", str,len);
		if (ret < 0)
			ret = alg_enum_search_ppfix(&oakley_group_names, "OAKLEY_GROUP_", " (extension)", str, len);
	}
	return ret;
}

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

In programs/pluto/ikev2.c near line 292:

It looks as if a packet that has both I and R flags, or neither, is
accepted.  Is this reasonable?


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

The comments in programs/pluto/ikev2_crypto.c line 107 are scary.

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

programs/pluto/spdb_struct.c line 206:

I think that it is generally good style to use braces on neither or
both legs of an if.  Using it on one and not the other is correct but
less clear.