Re: [Ipsec-tools-devel] Patch to Add Policy Priority Support

[Brian B. <bbu...@qu...>]

Aidas,

> thanks for fixes. They are in CVS. I also updated and rearanged litle 
> bit man pages to reflect changes in fixes. It would be good if someone 
> could take a look at english and at man page coding, as I'm not strong 
> at these.

Thanks for updating the man pages. A few comments about them:

ipsec_set_policy.3
"Policy position is determined by a signed 32-bit integer specifying the 
offset from the default priority with positive offsets indicating higher 
priorities (policy is placed closer to the beginning of the list) and 
negative offsets indicating lower priorities (policy is placed closer to 
the end of the list)."

I'm not sure sure we want to say 32-bit integer offsets here since the 
full range of 32-bit ints is not allowed when it is an offset from a 
base. Furthermore, the offset can be from the default priority but from 
one of the other ones (ie. "low" or "high"). I'd suggest it read as the 
following:

"Policy position is determined by a signed 32-bit integer where higher 
priorities indicate the policy is placed closer to the beginning of the 
list and lower priorities indicate the policy is placed closer to the 
end of the list."

"at the end of group of such policies" should be changed to "at the end 
of the group of such policies"

"It could be up to" should be changed to "It can be up to"

"to negative" should be changed to "for negative offsets."

"Policy position is determined by sum of base and offset."

This sentence is somewhat confusing since it may be a sum or a 
difference depending on the operator before the offset. I don't think we 
really need this sentence as the syntax should be pretty self explanatory.

"Policy priority interpretation in these functions and in kernel DO 
differ. Relationship between the two could be described as p(xfrm) = 
0x80000000 - p(func)" should be changed to "The interpretation of policy 
priorities in these functions and the kernel DOES differ. The 
relationship between the two can be described as p(kernel) = 0x80000000 
- p(func)"

Overall, I'm not so sure we need to give the hex values for low, def, 
and high since the user shouldn't really be concerned about them. I also 
don't think we should give the hex values for the maxes and mins since 
they can not actually be input.

Other than the above, the man pages look ok to me.

>
>> The reason I chose to special case the handling of priority 0 is so 
>> that you wouldn't have such large offsets if they were inserted with 
>> an old setkey or the kernel did not support it.
>>
>> Also, I'm not so sure what you meant when you said the policies would 
>> be shown equally. Can you please clarify that? 
>
>
> I see, I was wrong.
>
>> Policies inserted with an old setkey or with a kernel that does not 
>> support priorities should be output in the same manner they are in 
>> older versions of setkey. Only policies with a priority of non-zero 
>> will be output in the new format with an offset from one of the 
>> constants.
>
>
> Speaking of backwards compatibility, I think that priorities 
> implemented one way or another will not have backward compatibility 
> good enought and Ludvik will not allow them to go into 0.3.x. Taking 
> into account issue of default OpenSSL certificate use, I think we will 
> need to start 0.4.x anyway. Therefore we are more free to decide how 
> that backward compatibility should be broken.
>
> I would suggest to have no "prio def" displayed at all when priority 
> is default. Why? Because in that case typical user (who still do not 
> need priorities) will upgrade, flush and reload SPD, restart racoon 
> and will notice NO CHANGE AT ALL while inspecting or inserting rules.

Ok. That is fine with me if others agree.

>
> Displaying old/non supported priorites is special case anyway. Why 
> couldn't we display "prio old" or "prio max" in these cases? If we add 
> "max" as a legal priority, then user will not need to enter that ugly 
> number (insert at kernel's priority 0 when kernel supports priorities) 
> when software is upgraded and SPD is not [completely] yet. [[ Sorry 
> for not saying this earlier, but that idea come into my head just few 
> minutes ago.]]

Displaying "prio max" might be ok, but I don't think it should be added 
as a legal priority because it could be misleading. Max might be 
interpreted as inserted at the very beginning of the list while that may 
not actually be the case (ie. when there is already a policy with "prio 
max"). If people don't think that max is too confusing, and we make sure 
to state in the manpage that max does not necessarily imply the very 
beginning of the list, then this too could be added.

Brian