> On Apr 20, 2016, at 9:35 AM, Xuelei Fan <xuelei....@oracle.com> wrote: > > On 4/20/2016 9:17 AM, Wang Weijun wrote: >> >>> On Apr 20, 2016, at 7:41 AM, Xuelei Fan <xuelei....@oracle.com> wrote: >>> >>> On 4/19/2016 11:41 PM, Wang Weijun wrote: >>>>>>>> http://cr.openjdk.java.net/~weijun/8051408/webrev.10/ >>>>>> >>>>>> Please update copyright dates. >>>>>> >>>>>> src/java.base/share/classes/java/security/Provider.java >>>>>> ------------------------------------------------------- >>>>>> 145-151: Looks like the comment are not correct. There are >>>>>> getInstance(alg,params) since JDK 1.4 (See CertStore) >>>> Yes, but in those cases, there is either getInstance(alg) or >>>> getInstance(alg,params), but not both. In SecureRandom, we are now >>>> supporting both, and a fallback is needed for those implementations that >>>> does not override new SecureRandomSpi(SRP). >>>> >>>>>> >>>>>> 156-157: Fallback change the behavior significantly. Construct with or >>>>>> without parameter can be very differently. For example, LDAP cert store >>>>>> get requested, but the CertStore.getInstane(String, >>>>>> LDAPCertStoreParameters) may not return an LDAP cert store. Can you >>>>>> make more comment why the fallback is OK? >>>> For CertStore (and Configuration and Policy), all of its implementations >>>> only support the with-arg constructor, >>> I think the constructor is not of CertStore, but of CertStoreSpi >>> implementation. >> >> Yes. >> >>> The provider implementation may support any kind of >>> constructor (implicit or explicit, intended or not). The constructor >>> should be unknown to Provider. >> >> Not sure what you mean, but Provider uses reflection to get the constructor >> and create new instances. By convention, when something is created via >> getInstance(alg), the SPI class would provide the arg-less constructor; when >> via getInstance(alg,params), it would provide the with-arg constructor. >> > Not sure what you mean. > > public MyCertStore extends CertStoreSpi { > > public MyCertStore() { > // whatever > // ;-) Don't ask me why this construct is necessary. > } > > public MyCertStore(XXX params) { > // throws NoSuchMethodException > // ;-) Don't ask me why throw this exception. > } > } > > newInstanceUtil(MyCertStore, ...) > > The MyCertStore() would get called, unexpectly. Am I missing something?
Probably not, unless you call getInstance(arg, null). I am not sure this null will trigger some other exception along the way. OK, I admit there is a side effect here: If you design getInstance(alg,params) but params is always null, then you can only implement a constructor with no params. This is stupid and useless, but not really harmful. > >>> >>>> so NoSuchMethodException will never be thrown. Again, this fallback is >>>> only for SecureRandom now. >>>> >>>> >>> Please add more comment and control so that this only apply to SecureRandom. >> >> It already had "This is to support the enhanced SecureRandom". >> > My understanding is: this is to support the enhanced SecureRandom, and > it now applies to non-SecureRandom too. > > If it does not apply to non-SecureRandom, please add comment and update > the code accordingly. Comment added. As described above, there should not practical impact on non-SecureRandom primitives. I would revert the following line 159 throw new IllegalArgumentException(nsme); back to a simple "throw nsme". Thanks Max > > Xuelei
