Looks good Brad. One comment :
You might hit an NPE here but I guess the Exception handling in the
parent call would handle it :
JceSecurity.java :
254 if (Paths.get(cryptoPolicyProperty).getNameCount() != 1) {
Maybe add a null check to be cleaner ?
I missed the "crypto.policy=crypto.policydir-tbd" approach in 1st
revision. Isn't it going to confuse people studying the code ? This
value gets replaced at build time - right ? What about changing it to
something like :
"crypto.policy=$crypto.policydir-tbd //replaced at build time"
and having the make files search for that string instead ? I think it
makes the intent more obvious.
Regards,
Sean.
On 25/08/2016 01:21, Bradford Wetmore wrote:
Max/SeanC/SeanM,
The latest update:
http://cr.openjdk.java.net/~wetmore/8061842/webrev.02/
On 8/17/2016 9:26 PM, Wang Weijun wrote:
Before this change, you require default policy in neither export nor
import to be empty but do not care about the getMinimum() result. In
this change, you make sure the final result is not empty. I assume
this is a fix?
I made the change to allow for our traditional (default) export/import
mechanism, but other additional styles could be added/used. Since we
no longer sign, distros are free to edit, add and/or remove files.
But before doing any JCE operation, the environment needs to grant
something, otherwise there are no perms and no JCE available.
283 // Did we find a default perms?
What does this line mean?
I've moved to the right position in the file. I meant did we find a
default perms file, vs an exempt.
296 // This should never happen
But you can still print out the file name.
I'm concerned that the exception might print out the entire path
instead of just the filename, which would include java.home and
probably should not be made available.
Can you rename policydir-tbd to something else? I am afraid it will
be confused with policy.url.1 etc.
Changed to: crypto.policydir-tbd?
The original README.TXT in unlimited says "are exportable from the
United States." and you have "is exportable." now. Is this intended?
(IANAL)
Changed.
TestUnlimited.java:
45 "// Use the AES are the test Cipher", you mean "Use AES as the
test Cipher"?
51 "throw new Exception ("Unlimited policy is NOT active");". No
space before "(".
Fixed.
Sean Mullan wrote:
> What about setting the default value to "limited"? And then this
> would only be changed to "unlimited" if the build --enable-unlimited-
> crypto option is specified?
I could, but I'm concerned that a build with
--enabled-unlimited-crypto would expect that the compiled-in version
default would also be unlimited and would be surprised with limited.
Upon Max's suggestion above, I've changed the name of the marker to
"crypto.policy=crypto.policydir-tbd." Does that work for you?
> Instead of throwing an exception here, I wonder if it would make more
> sense to assume a default value of "limited" if the property is not
> set or is empty.
We could, but see above.
Sean Coffey wrote:
> Please include the exception 'e' in your last exception here.
Again, I'm concerned about outputting java.home, so I'm just going to
output the final directory name.
> 3. Test case.
>
> The TestUnlimited.java testcase seems to be lacking. Do you want to
> test other values for crypto.policy ? 'limited' would be one.
> Throwing in some dummy value would also be good so that the exception
> handling code gets exercised.
Done.
* @run main/othervm TestUnlimited limited fail
* @run main/othervm TestUnlimited unlimited pass
* @run main/othervm TestUnlimited NosuchDir exception
* @run main/othervm TestUnlimited . exception
* @run main/othervm TestUnlimited /tmp/unlimited exception
* @run main/othervm TestUnlimited ../policy/unlimited exception
* @run main/othervm TestUnlimited ./unlimited exception
> It needs to be run in ovm mode since you're setting a Security
> property.
Yes, good catch.
Brad
