<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Hi Brad, <br>
</p>
<p>nice to have this going in. Some comments.</p>
<p>1. Bug synopsis, can you edit it perhaps. "Introduce security
property to control strong crypto" seems more descriptive.</p>
<p>2. Exception handling.</p>
<p>Alot of your new exceptions don't include context. That makes
debugging more difficult than needs be.</p>
<p>
<blockquote type="cite">
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; white-space: pre-wrap;">+ throw new SecurityException(
+ "Invalid cryptographic jurisdiction policy directory value");</pre>
</blockquote>
<blockquote type="cite">
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; white-space: pre-wrap;">+ if (!Files.isDirectory(path) || !Files.isReadable(path)) {
+ throw new Exception("Can't read cryptographic policy directory");</pre>
</blockquote>
<br>
<blockquote type="cite">
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; white-space: pre-wrap;">+ throw new SecurityException(
+ "Unexpected jurisdiction policy filename found");</pre>
</blockquote>
</p>
<p>
<blockquote type="cite">
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; white-space: pre-wrap;">+ } catch (Exception e) {
+ throw new SecurityException(
+ "Couldn't parse jurisdiction policy files");</pre>
</blockquote>
Please include the exception 'e' in your last exception here.</p>
<p><br>
</p>
<p>
<blockquote type="cite">
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; white-space: pre-wrap;">+ } catch (DirectoryIteratorException ex) {
+ // I/O error encountered during the iteration,
+ // the cause is an IOException
+ throw new SecurityException(
+ "Couldn't iterate through the jurisdiction policy files");
+ }</pre>
</blockquote>
Can you add the DirectoryIteratorException to the final exception
that you're throwing ?</p>
<p>3. Test case.</p>
<p>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.</p>
<p>It needs to be run in ovm mode since you're setting a Security
property.<br>
</p>
<pre class="moz-signature" cols="72">Regards,
Sean.</pre>
<div class="moz-cite-prefix">On 18/08/2016 05:26, Wang Weijun wrote:<br>
</div>
<blockquote
cite="mid:151C5E71-0DC6-4A93-8C2D-36EA599360A5@oracle.com"
type="cite">
<pre wrap="">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?
283 // Did we find a default perms?
What does this line mean?
296 // This should never happen
But you can still print out the file name.
Can you rename policydir-tbd to something else? I am afraid it will be confused with policy.url.1 etc.
The original README.TXT in unlimited says "are exportable from the United States." and you have "is exportable." now. Is this intended? (IANAL)
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 "(".
No other comment.
--Max
</pre>
<blockquote type="cite">
<pre wrap="">On Aug 18, 2016, at 7:22 AM, Bradford Wetmore <a class="moz-txt-link-rfc2396E" href="mailto:bradford.wetmore@oracle.com"><bradford.wetmore@oracle.com></a> wrote:
New webrev:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8061842">https://bugs.openjdk.java.net/browse/JDK-8061842</a>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~wetmore/8061842/webrev.01/">http://cr.openjdk.java.net/~wetmore/8061842/webrev.01/</a>
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>