Code Review Request for 7146728 and 7130959

[Valerie (Yu-Ching) Peng]

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
>>