<div dir="ltr"><div dir="ltr"><div class="gmail_extra"><div class="gmail_extra">Hi Valerie,</div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks for your feedback.</div><div class="gmail_extra"><br></div><div class="gmail_extra">I suggest to keep only one hierarchy of webrevs, so I'll continue in Webrev.12 using your Webrev.02 as a base.</div><div class="gmail_extra"><br></div><div class="gmail_extra">In general, I'm happy with your P11Key refactorings and with keeping the previous reference inc/dec scheme on P11key clients side.</div><div class="gmail_extra"><br></div><div class="gmail_extra">I'll go file-by-file reviewing changes between your Webrev.02 and my Webrev.11:</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11Cipher.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra">   * I used to set "initialized" variable to false in all "initialize" function error flows but it's not necessary as it shouldn't be true when entering.</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11DHKeyFactory.java</div><div class="gmail_extra">  * Copyright updated</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11DSAKeyFactory.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11Digest.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11ECDHKeyAgreement.java</div><div class="gmail_extra">  * Copyright updated</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11ECKeyFactory.java</div><div class="gmail_extra">  * Copyright updated</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11Key.java</div><div class="gmail_extra">  * Why is P11Key now public?</div><div class="gmail_extra">  * I have some concerns regarding how you calculate "extractKeyInfo" value:</div><div class="gmail_extra">   * Why is !sensitive a requirement?</div><div class="gmail_extra">    * We can extract them wrapped, and the code already handles that.</div><div class="gmail_extra">   * !tokenObject should be a requirement</div><div class="gmail_extra">    * I know it's checked in NativeKeyHolder constructor but it could be part of extractKeyInfo too for clarity</div><div class="gmail_extra">   * Why is !type.equals(SECRET) a requirement?</div><div class="gmail_extra">  * It's correct that we can not destroy the key after creation (under the assumption that will be used soon), and that this should be a performance benefit for most cases. My approach here was more memory conservative, but it should be fine anyways. I'm okay with this change.</div><div class="gmail_extra">  * In case of createNativeKey failure, I think we should decrement the previously-incremented reference counter</div><div class="gmail_extra">  * Why do we need refCount to be of AtomicInteger type? All the modifications to this instance variable are done inside instance synchronized methods (getKeyID and releaseKeyID)</div><div class="gmail_extra">  * The benefit of the previous incNativeKeyRef/decNativeKeyRef methods is that they don't pay a sync cost for all cases in which the feature is disabled. I believe we can keep that performance improvement here too.</div><div class="gmail_extra">   * nativeKeyInfo variable is written at creation time and all other usages are read-only, so we can have multiple readers at the same time and synchronize on it if not null (instead of synchronizing the whole method). What do you think?</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11KeyAgreement.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11KeyGenerator.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11KeyPairGenerator.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11KeyStore.java</div><div class="gmail_extra">  * Copyright updated</div><div class="gmail_extra">  * The reason why a call to "makeNativeKeyPersistent" was made in P11KeyStore.updateP11Pkey method was because "P11Key key" could have been extracted and the change not persisted in the keystore. This call was disabling the feature for such keys. However, we now decided -for a good reason- that keys from keystores (token objects) will not be extracted. Thus, this call is no longer needed. I'm okay with this change.</div><div class="gmail_extra">  * "long newKeyID" variable not used. Fixed on some other files too.</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11Mac.java</div><div class="gmail_extra">  * Copyright updated</div><div class="gmail_extra">  * Split ensureInitialized and initialize functions again for the reasons we've previously discussed.</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11RSACipher.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11RSAKeyFactory.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11SecretKeyFactory.java</div><div class="gmail_extra">  * Copyright updated</div><div class="gmail_extra">  * The inc/dec pattern was broken in case of "token.p11.C_CopyObject" failure. Fixed.</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11Signature.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11TlsKeyMaterialGenerator.java</div><div class="gmail_extra">  * Removed dead & debug code</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11TlsMasterSecretGenerator.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11TlsPrfGenerator.java</div><div class="gmail_extra">  * Removed dead & debug code</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * P11TlsRsaPremasterSecretGenerator.java</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * PKCS11.java</div><div class="gmail_extra">  * Copyright updated</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * p11_keymgmt.c</div><div class="gmail_extra">  * Removed dead code</div><div class="gmail_extra">  * I've seen that you replaced some variable assignments with memcpy and memset calls because of compiler errors. I wonder what these errors are, as we should be able to cast and do assignments.</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * pkcs11t.h</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * pkcs11wrapper.h</div><div class="gmail_extra">  * Ok</div><div class="gmail_extra"><br></div><div class="gmail_extra">In addition, I removed trimming white spaces in several files.</div><div class="gmail_extra"><br></div><div class="gmail_extra">I agree with having a property to completely disable this feature. I've not seen, so far, unfixed cases in which extraction succeeds but re-creation fails.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Webrev.12 is based on your Webrev.02, with modifications previously described:</div><div class="gmail_extra"><br></div><div class="gmail_extra"> * <a href="http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12/">http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12/</a></div><div class="gmail_extra"> * <a href="http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12.zip">http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12.zip</a></div><div class="gmail_extra"><br></div><div class="gmail_extra">NOTE: Webrev.12 does not address neither the P11Key.java issues above nor the property to completely disable this feature. Those 2 things are the only pending on my side for now.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Kind regards,</div><div class="gmail_extra">Martin.-</div></div></div></div>