On 17 Dec 2014, at 18:59, Sean Mullan <sean.mul...@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.mul...@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
