On Tue, 30 Nov 2021 19:48:19 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Hmm, thinking more about "internal"/"opaque", given this is naming for the >> parent, maybe "internal" is more correct. The non-sensitive keys are >> encapsulated by the children classes and is still an instance of the parent. >> If you name the parent class w/ "opaque" suffix, it looks >> misleading/confusing. The opaqueness is implied by the implementation >> instead of the properties of the objects. > >> Hi @valeriepeng , >> >> Yes, I just verified how serialization works for P11Keys and your >> 'transient' change makes sense to me now. >> >> I see your point about Internal/Opaque. Keep 'Internal' then if you prefer. >> The whole inheritance relationship between these classes sounds a bit weird >> to me, beyond the names we call them. I wouldn't suggest any additional >> changes there now, though. >> >> It looks to me that the only 2 changes expected for Webrev.02 are: 1) >> P11RSAPrivateNonCRTKey to use the parent's protected 'n'; and 2) removal of >> 'long errorCode = e.getErrorCode();'. >> >> Now that we almost have the changeset ready, I'm not sure of how to proceed >> with the push. Do you want me to commit Webrev.02 in my own branch and use >> the 'Co-authored-by:' header? If we do that, do we still need an additional >> Reviewer? Do you prefer to have your own branch? Please let me know of how >> to move forward. >> >> Martin.- > > You can just incorporate the changes on your branch and proceeds with me > being the reviewer. The webrev that I sent u is just an easier way to express > the comments/feedbacks. > > Thanks, > Valerie Hi @valeriepeng I've reverted the initial 2 changesets on this branch, rebased to the latest commit on 'master' (git merge), applied Webrev.02 as discussed and added the test case from the initial commit (with the change from the 2nd commit applied on top). I've noticed no regressions in jdk/sun/security/pkcs11. Does it look good to you? Thanks, Martin.- ------------- PR: https://git.openjdk.java.net/jdk/pull/4961
