Code Review Request for 6953295 and related changes to add keytool to jdk.tools.base
mandy.chung at oracle.com
Wed Sep 14 16:14:05 PDT 2011
On 09/14/11 12:08, Sean Mullan wrote:
> Requesting review from Mandy and Max, others welcome.
> CR: http://monaco.us.oracle.com/detail.jsf?cr=6953295
> Webrev: http://cr.openjdk.java.net/~mullan/jigsaw/webrevs/6953295/webrev.00/
L2079, 2336: keytool -sslserver option and -file ldap://.... option
are now optional. Should keytool output an error if these options are
used but class not found? Such detection can be done at startup.
L2073, 2330: Besides these places, I also found that
sun.security.provider.certpath.URICertStore.LDAP does similar work.
Would it be better to add a factory class that returns the
CertStoreHelper of a given type if present or null if not (e.g.
sun.security.provider.certpath.CertStoreHelperFactory?) ? This would
avoid hardcoding the implementation class name (as a string) in many places.
L2064-2323: there are a couple of places that can be updated with
try-with-resource. Since you're in this file, it might be good to
modernize part of the existing code (nice to see the change to use the
L2344: CertStore.getCertifications never returns null. Looks like
L2355-2362 is to handle the case at L2347. If that's the case, can you
move L2356-2361 to L2347?
> Additional notes to reviewers:
> * I will push the non-module related changes to JDK 8 first. I wanted to first
> make sure everything worked with the module changes.
Thanks. Any change for jdk modularization not depending on jigsaw will
be in jdk 8 repos where other jdk8 development works are based on.
> * The following classes have been moved to the sun.security.pkcs10 package to
> remove dependencies on the sun.jsse module:
> and these classes have been moved to the sun.security.tools package:
> The code is the same other than changes due to the package rename.
> * The following classes have been removed since nothing in the JDK uses them:
We all like removing dead code!
> The test/sun/security/util/BigInt/BigIntEqualsHashCode.java was also removed as
> it is now N/A.
> * Should I also include jarsigner in jdk.tools.base? I think it would be useful
> for creating signed modular jars.
I think it's useful too. With this fix, it should be fine including
jarsigner in the jdk.tools.base module. It was not included previously
because of its dependency on KeyTool that in turn depends on jsse and jndi.
jdk.tools.base is also intended to aggregate the developer tools that
are needed for module development with only the base module so that it
can be installed at once rather than installing each one individually.
More information about the jigsaw-dev