Sorry, for the delayed response, I have been working in another area.
Comments below:
On 11/29/2012 12:49 PM, Sean Mullan wrote:
Hi Steve,
Most of this looks good. Here are my comments:
* General
You haven't added any new regression tests for this. Since this is
essentially a lot of code refactoring and covered by existing tests, can
you add the noreg-cleanup keyword to the bug?
When I figure out the new bug system, will do this.
* src/share/classes/java/security/AlgorithmParameters.java
[394] You can't make this change, since it would violate the spec which
says it returns null. You will need to workaround this some other way.
I will revert the change.
* src/share/classes/sun/security/ec/ECDSASignature.java
[278, 305] The comment should be aligned to the left.
OK
* src/share/classes/sun/security/pkcs11/P11ECKeyFactory.java
[121-2] The exception message here seems misleading, since it doesn't
have to be an instance of ECPublicKey, it could just be a PublicKey as
long as the encoding is correct. Why not just set the cause to the
InvalidKeySpecException, ex:
OK, I will change the code to use the InvalidKeySpecException from the
key factory to create a InvalidKeyException.
Sorry, I keep forgetting about the cause constructor on exceptions,
because for 8 years while working on J2ME MIDP I did not have cause
constructor on exceptions or the getCause method.
What exception message did the old code throw? It might be best to
preserve that behavior.
[151-2] same comment as above
OK
* src/share/classes/sun/security/util/ECUtil.java
[230-60] Can you add a comment as to why this is commented out?
This commented out code was in ECParameters that was not need to
implement the public contract, but to be utility code like encodePoint
and decodePoint, so I moved it *as is* to ECUtil.
I would like to remove it, as it the policy of other groups, but I don't
know your groups policy.
Which would you like: remove the code or add the comment "Code that will
have a possible use in the future"?
Steve.
--Sean
On 11/26/2012 10:21 PM, Stephen Flores wrote:
Vincent, Sean,
Please review the fix for:
CR 7194075: Various classes of sunec.jar are duplicated in rt.jar
http://cr.openjdk.java.net/~sflores/7194075/webrev-1/
Changes:
*Changed/renamed any of methods that did not support the public API to
package private.
*Moved the decode and encode point methods out of ECParameters to a new
class sun.security.util.ECUtil.
*Changed any "new byte[], System.arraycopy" blocks in ECUtil point
methods to Arrays.copyOfRange.
*Added a new AlgorithmParameterSpec in sun.security.util to get curves
by key size, for PKCS11 to use.
*Moved all of static lookup methods in ECParameters, NamedCurve and the
curve repository to separate class (CurveDB). This made ECParameters and
NamedCurve cleaner and easier work on (there was some ECParameters
cleanup.
*In JSSE and PKCS11 and changed the references to ECParmeters and
NamedCurve to the ECUtil which has utility methods that use the public
APIs.
*Changed to the EC unit test to use the list of supported curves in the
property that the SunEC provider has already.
*Changed SunECEntries to build the list of supported curves property
from the collection in CurveDB.
*Changed the JDK makefiles to not duplicate EC classes in rt.jar.
Thanks,
Steve.
