Code Review Request for 6953295 and related changes to add keytool to jdk.tools.base

[Mandy Chung]

  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/
>

KeyTool.java
    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 
foreach)  :)

    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:
>
> sun.security.pkcs.PKCS10	
> sun.security.pkcs.PKCS10Attribute
> sun.security.pkcs.PKCS10Attributes
>
> and these classes have been moved to the sun.security.tools package:
>
> sun.security.x509.CertAndKeyGen
> sun.security.util.PathList
>
> 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:
>
> sun.security.pkcs.EncodingException
> sun.security.util.BigInt
>

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.

Mandy