[9] request for review 8044445: Create PKCS12 Keystores by Default

[Vincent Ryan]

FYI I’ve updated the webrev to include the changes below:
    http://cr.openjdk.java.net/~vinnie/8044445/webrev.05/


On 17 Dec 2014, at 20:08, Vincent Ryan <vincent.x.ryan at oracle.com> wrote:

> On 17 Dec 2014, at 18:59, Sean Mullan <sean.mullan at oracle.com> wrote:
> 
>> On 12/16/2014 07:12 PM, Vincent Ryan wrote:
>>> I’ve generated a new webrev to address your comments below:
>>>    http://cr.openjdk.java.net/~vinnie/8044445/webrev.04/
>>> 
>>> Thanks.
>>> 
>>> 
>>> On 16 Dec 2014, at 21:11, Sean Mullan <sean.mullan at oracle.com> wrote:
>>> 
>>>> Here are my comments on the latest webrev:
>>>> 
>>>> * General
>>>> 
>>>> - for the engineProbe methods, if a DataInputStream is not specified, I think you should wrap the stream in a DataInputStream, rather than returning false.
>>> 
>>> KeyStore.getInstance(File,…) always supplies a DataInputStream to KeyStoreSpi.engineProbe
>>> so it is not necessary to wrap.
>> 
>> That's an implementation detail though. Another vendor's implementation of the new KeyStore.getInstance methods may not do that. If you want to guarantee that, you should change the InputStream parameter of the engineProbe method to a DataInputStream. Unless you think engineProbe may be called/used in other circumstances where a DataInputStream is not appropriate, which in that case you change the subclasses to do an instanceof and  wrap it.
> 
> OK that makes sense. I’ll wrap it.
> 
> 
>> 
>>>> * keytool/Main
>>>> 
>>>> - can you explain more about why the provider is ignored on lines 812-817, 1919-1925? This doesn't seem like it is compliant with keytool -- if a provider is specified, it should be used.
>>> 
>>> The probing mechanism is better equipped to determine the exact keystore type.
>>> 
>>> For example, if an application specifies no -storetype option then it implicitly gets the
>>> default keystore type which now differs between JDK 9 and earlier releases.
>>> The new compatibility mode can handle this scenario so that the application doesn’t
>>> break. However, in compatibility mode the instantiated keystore type might not match
>>> the keystore format.
>>> 
>>> Whereas when probing is employed the instantiated keystore type always matches
>>> the keystore format.
>> 
>> Ok, but I think you should do that only if both a provider and storetype are not specified. If the -providerName option is specified, you need to honor that. It looks like the code is not doing that right now.
> 
> Sorry, I misread your previous comment. I thought you were asking why the storetype is ignored.
> I hadn’t considered the impact of the -providerName option. I’ll make that change.
> 
> 
>> 
>> --Sean
>