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

[Sean Mullan]

New webrev: http://cr.openjdk.java.net/~mullan/jigsaw/webrevs/6953295/webrev.01/

Comments on your comments below -

On 9/14/11 7:14 PM, Mandy Chung wrote:

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

It will output an error if the classes are not found indicating that SSL or LDAP
was not available. I could probably move the check earlier but I'm not sure that
is going to make much of a noticeable difference.

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

Good point. I have added a getInstance method to CertStoreHelper and it will
load the implementations on demand and store them as SoftReferences.

>     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)  :)

Yes, I converted as many as I could to try-with-resource although there were a
few with some complicated logic that I left alone for now. I also converted a
few others in URICertStore and also changed some others to use multi-catch.

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

Good catch - you caught a bug in my code. Actually, an SSL server may return a
null or empty chain, and I want to preserve the current behavior of KeyTool
which treats this as an error. So I've modified the SSLServerCertStore to return
an empty chain in this case.

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

In the previous webrev, I used "hg remove" and "hg add" to move these files. I
reverted those changes and used "hg rename" instead, so the diffs for these
files should be much easier to review.

>> * 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.

I have modified the module config files to include jarsigner in jdk.tools.base.

--Sean