Code Review Request for 7146728 and 7130959

[Valerie (Yu-Ching) Peng]

Brad,

As I was preparing the files for putting back, I noticed some 
now-redundant code in DHKeyAgreement2.java as well as one minor issue 
about resetting the key agreement object in the 
engineGenerateSecret(...) method of DHKeyAgreement.java.

Hope this to be the last version of the webrev:
http://cr.openjdk.java.net/~valeriep/7146728/webrev.04/

Please let me know if you have any more comment.
Thanks!
Valerie

On 03/14/12 16:54, Valerie (Yu-Ching) Peng wrote:
> Sure, that's good clarification.
> I'll add the suggested wording and put back the changes.
> Thanks,
> Valerie
>
> On 03/14/12 16:01, Brad Wetmore wrote:
>> Here's the comment I was trying to suggest in your office yesterday:
>>
>>         /*
>>          * NOTE: BigInteger.toByteArray() returns a byte array 
>> containing
>>          * the two's-complement representation of this BigInteger with
>>          * the most significant byte in the zero-th element. This
>>          * contains the minimum number of bytes required to represent
>>          * this BigInteger, including at least one sign bit whose value
>>          * is always 0.
>>          *
>>          * Keys are always positive, and the above sign bit isn't
>>          * actually used when representing keys.  (i.e. key = new
>>          * BigInteger(1, byteArray))  To obtain an array containing
>>          * exactly expectedLen bytes of magnitude, we strip any extra
>>          * leading 0's, or pad with 0's in case of a "short" secret.
>>          */
>>
>> Hope this helps,
>>
>> Brad
>>
>>
>>
>> On 3/9/2012 12:57 PM, Valerie (Yu-Ching) Peng wrote:
>>>
>>> Ok, more comments/changes added as suggested.
>>> Webrev updated at:
>>> http://cr.openjdk.java.net/~valeriep/7146728/webrev.02/
>>>
>>> Thanks,
>>> Valerie
>>>
>>> On 03/08/12 17:15, Brad Wetmore wrote:
>>>>
>>>> >> DHKeyAgreement:
>>>> >> ===============
>>>>
>>>> > I added more comments to both code blocks to explain what's going 
>>>> on.
>>>>
>>>> A couple more requested comments.
>>>>
>>>> Around line 299, could please add that the sign bit is always 0.
>>>>
>>>> Around line 303, could you put in a comment that since there is enough
>>>> room here for the sign bit in this array, it just becomes a leading 
>>>> zero.
>>>>
>>>>> Good point, I modified the engineGenerateSecret() to leverage
>>>>> engineGenerateSecret(byte[], int) method.
>>>>
>>>> Great, thanks!
>>>>
>>>>>> P11KeyAgreement.java
>>>>>> ====================
>>>>>>
>>>>>> // Solaris
>>>>>>
>>>>>> Shouldn't this be "// Solaris/NSS", for the case in which there 
>>>>>> isn't
>>>>>> a leading zero?
>>>>> Well, I think putting NSS in both places seems somewhat confusing. 
>>>>> What
>>>>> I tried to point out is that NSS (at least in the older versions, 
>>>>> I have
>>>>> not tried the newer version to see if they switched behavior) 
>>>>> trims off
>>>>> the leading 0s whenever possible. I've made some modification 
>>>>> hoping to
>>>>> make it clearer.
>>>>
>>>> Much better, thanks!
>>>>
>>>> Minor nit (take or leave) you could adjust the logic around 208 to
>>>> only create newSecret if secret.length < secretLen.
>>>>
>>>> if (secret.length > secretLen) {
>>>> throw new ProviderException(...
>>>> }
>>>> byte [] newSecret = new byte[secretLen];
>>>> System.arraycopy(...);
>>>> return newSecret;
>>>>
>>>>>
>>>>
>>>>>> TestShort.java
>>>>>> ==============
>>>>>> New bugid
>>>>>>
>>>>>> @bug 4942494 7146728
>>>>> Done.
>>>>>
>>>>>> Indention problem around the try.
>>>>> Fixed.
>>>>
>>>> Thanks for taking out the commented out code, I almost mentioned it
>>>> and then didn't.
>>>>
>>>> Brad
>>>
>