On Thu, 18 Nov 2021 18:37:38 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>>> > ``` >>> > * By eliminating P11RSAPrivateKey::getModulus, looks to me that >>> > P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now >>> > called, leading to an unnecessary call to the native library as the >>> > modulus was already received on P11RSAPrivateKey constructor. This >>> > happens to P11RSAPrivateNonCRTKey as well. >>> > ``` >>> >>> There shouldn't be another call to the native library as it is only made >>> when the modulus n is null. However, since n is already available, >>> overriding the getModulus() method makes sense as there is no need to call >>> fetchValues() which should return upon a non-null n value, but still an >>> overhead. >>> >> >> In my view (Webrev.00 based comment), the variable 'n' holding the modulus >> value is private in P11RSAPrivateKey. This means that when >> P11PrivateKeyRSA::getModulus is called, P11PrivateKeyRSA::n (which is >> protected) has a 'null' value and the PKCS#11 lib call is done again. > >> >> >> > > ``` >> > > * By eliminating P11RSAPrivateKey::getModulus, looks to me that >> > > P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now >> > > called, leading to an unnecessary call to the native library as the >> > > modulus was already received on P11RSAPrivateKey constructor. This >> > > happens to P11RSAPrivateNonCRTKey as well. >> > > ``` >> > >> > >> > There shouldn't be another call to the native library as it is only made >> > when the modulus n is null. However, since n is already available, >> > overriding the getModulus() method makes sense as there is no need to call >> > fetchValues() which should return upon a non-null n value, but still an >> > overhead. >> >> In my view (Webrev.00 based comment), the variable 'n' holding the modulus >> value is private in P11RSAPrivateKey. This means that when >> P11PrivateKeyRSA::getModulus is called, P11PrivateKeyRSA::n (which is >> protected) has a 'null' value and the PKCS#11 lib call is done again. > > Hmm, this is a bug and unintended. The 'n' in the child class should be > removed as the 'n' in the parent class has scope protected and should be used > instead. > > I checked webrev.01 and this has been caught and fixed already. Good to have > a different pair of eyes and more likely to spot problems. Thanks! > > Regards, > Valerie Hi @valeriepeng , Some comments and questions regarding Webrev.01: * P11Key.java * Would you consider replacing the 'Internal' suffix with 'Opaque'? I believe the term 'opaque' better reflects what these keys are: you cannot see their values -thus, opaque- but you can use them as-is. 'Internal' gives me the impression of something not supposed to be exposed; and this is not exactly the case. * Why are P11RSAPrivateKeyInternal::n, P11RSAPrivateKey::<e, d, p, q, pe, qe, coeff>, etc. now transient? * Now that P11RSAPrivateKey::n private field was removed, the extra PKCS#11 lib call I mentioned in Webrev.00 is not possible anymore. The override of ::getModulus looks good to me, though, for the reasons you said. * Can P11RSAPrivateNonCRTKey use the protected 'n' field from its parent instead of declaring a private one? * P11Signature.java * 'long errorCode = e.getErrorCode();' -> Looks to me that this change was included into the Webrev by mistake, and is part of JDK-8236512. Thanks, Martin.- ------------- PR: https://git.openjdk.java.net/jdk/pull/4961
