Hi Valerie, All comments are good to me and accepted in the new webrev:
http://cr.openjdk.java.net/~xuelei/8072452/webrev.02/ On 4/13/2016 8:26 AM, Valerie Peng wrote: > Hi Xuelei, > > Mostly look good, just some comments in line... > > <src/java.base/share/classes/com/sun/crypto/provider/DHKeyPairGenerator.java> > > line 95-100, so essentially, we will use the built-in parameters as much > as possible and if none available, we will try to generate the > parameters for key sizes <= 1024, right? The current comment is a little > hard to parse. Maybe we can just explain it as below? > // Use the built-in parameters (ranging from 512 to 8192) when > available > this.params = ParameterCache.getCachedDHParameterSpec(keysize); > // Due to performance issue, only support DH parameters generation > // up to 1024 bits > if ((this.params == null) && (keysize > 1024)) { > Updated. > <src/java.base/share/classes/sun/security/provider/DSAParameterGenerator.java> > > - line 212, redundant? Yes. Updated. > <src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java> > > - line 277-303, the "params"argument for this particular method is > always null for non-RSA algorithms based on the usage. It is commented > on line 232 and several other places as indicated by the "// XXX sanity > check params" comments. So, more code changes will be needed here. > Either you can add another boolean hasParams and rely on that argument, > or change the specified argument type from RSAKeyGenParameterSpec to a > generic one and then cast it later when it is needed. > Good catch! Updated per the latter proposal. > <src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java> > - line 141 - 146 and line 1548 - 1554 are identical. Seems to me that > they can be customized a little as they are for different variable > assignment. The "customizedDHKeySize" variable on line 147 is from user, > right? Maybe what you really mean is something like the following? > // DH parameter generation can be extremely slow, best to use one of > the > // supported pre-computed DH parameters (see DHCrypt class) > As for line 1548 - 1554, it can be as simple as: > // DH parameter generation can be extremely slow, make sure to use > one of the > // supported pre-computed DH parameters (see DHCrypt class) Updated. > - line 148, can customizedDHKeySize be 1024? The old code accepts it but > the new code will error out. Is there a reason for rejecting 1024? The > check only rejects "<1024" but yet the exception message says the value > should be larger than 1024. > Lastly, "should larger" => "should be larger" (line 150). > 1024 bits should not be rejected. Updated the exception message. Thanks for the comments! Xuelei > I will look at the tests next and send u comments separately. > Thanks, > Valerie > On 4/1/2016 8:53 AM, Xuelei Fan wrote: >> Hi, >> >> Please review this improvement update to support DHE sizes up to >> 8192-bits and DSA sizes up to 3072-bits: >> >> http://cr.openjdk.java.net/~xuelei/8072452/webrev.00 >> >> This updated extends to support 3072-bits DH and DSA parameters >> generation, and pre-computed DH parameters up to 8192 bits and >> pre-computed DSA parameters up to 3072-bits. >> >> Thanks, >> Xuelei
