RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]

[Martin Balao]

On Tue, 30 Nov 2021 19:48:19 GMT, Valerie Peng <valeriep at 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