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 <[email protected]> wrote:
> On 17 Dec 2014, at 18:59, Sean Mullan <[email protected]> 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 <[email protected]> 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
>
