> On Apr 19, 2016, at 9:09 PM, Xuelei Fan <xuelei....@oracle.com> wrote:
>
> On 4/15/2016 9:
>> 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, so NoSuchMethodException will never be
thrown. Again, this fallback is only for SecureRandom now.
>
> src/java.base/share/classes/java/security/SecureRandom.java
> -----------------------------------------------------------
> 456 SecureRandomSpi spi = (SecureRandomSpi) instance.impl;
> 457 SecureRandom r = new SecureRandom(spi, instance.provider,
> algorithm);
> 458 return r;
>
> 456, there is a whitespace of the casting.
Will remove.
>
> The compiler may optimize the code, but I may use the old style as line
> 401-402.
>
> Similar comment for 507-510, 553-556.
You mean inline and remove the spi variable? Sure I can.
>
> 477 - secureRandomSpi.engineNextBytes(bytes);
> 667 + secureRandomSpi.engineNextBytes(
> 668 + Objects.requireNonNull(bytes));
> Some provider may tolerant null bytes input (for example SunPKCS11).
> Please consider this update more for compatibility.
Oh, then this will be unspecified. I'll think about it.
>
>
> src/java.base/share/classes/java/security/SecureRandomSpi.java
> --------------------------------------------------------------
> 124 throw new UnsupportedOperationException("not supported");
> 156 throw new UnsupportedOperationException("not supported");
>
> I would like to use no-parameter exception, or more specific message.
No params.
>
> 174 @Override
> 175 public String toString() {
> 176 return getClass().getSimpleName();
> 177 }
> What's the usage of this new toString() method?
Testing and debugging. For DRBG, it returns a text like the "drbg" security
property.
Thanks
Max
>
>
> Review the DRBG impl later.
>
> Thanks,
> Xuelei
>
>
>
