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

[Paul Wouters]

On Sun, 18 May 2008, D. Hugh Redelmeier wrote:

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

The comment in the comment says "aif AF=0 then value, if AF=1 then not present"

> - if a value field is present, it would not be constrained to be an
>   u_int_16_t.

That's true. I was not sure what to put in here, since the length has no
set boundary. The u_int16_t made it a multiple of 4.

I've changed the comment to:

        /* u_intXX_t isatr_value;      Value if AF=0, absent if AF=1 */

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

So far IKEv2 does not specify an option using this mechanism, but it does
specify this mechanism, which would be valid for private use options that
need to transfer a value larger then u_int_16_t (of the lv field when AF=0)

> ================================================================
> 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?

Yes.

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

I have made this change,

Paul