Code Review Request for 7146728 and 7130959

[Valerie (Yu-Ching) Peng]

Sure, thanks!
Valerie

On 03/19/12 20:15, Brad Wetmore wrote:
> Minor comment:
>
> // Array too short, pad it w/ leading 0s including the sign bit
>
> Can you pull off:  "including the sign bit"
>
> Looks good to me otherwise.
>
> Brad
>
>
>
>
>
> On 3/16/2012 1:42 PM, Valerie (Yu-Ching) Peng wrote:
>> 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
>>>>>
>>>
>>