[9] request for review 8046777: apple.security.KeychainStore has a problem searching for identities

[Vincent Ryan]

On 10 Nov 2014, at 18:13, Sean Mullan <sean.mullan at oracle.com> wrote:

> You missed one of my comments:
> 
> >> 623-629 similar comment as above

I made the change to join the two if statements into a single one.
Did you want me to use a local variable instead of casts?

> 
> Otherwise looks fine.
> 
> --Sean
> 
> On 11/10/2014 01:05 PM, Vincent Ryan wrote:
>> Thanks Sean. I’ve generated a new webrev that addresses your comments:
>>   http://cr.openjdk.java.net/~vinnie/8046777/webrev.01/
>> 
>> On 10 Nov 2014, at 16:50, Sean Mullan <sean.mullan at oracle.com> wrote:
>> 
>>> A few comments:
>>> 
>>> * KeystoreImpl.m
>>> 
>>> - it is odd to have a comment embedded in the parameter list and makes the line very long. Can you move this comment above the call?
>>> 
>>> - update copyright year
>>> 
>>> * KeychainStore.java
>>> 
>>> 297-304 I think it would be cleaner to write these lines as:
>>> 
>>>            } else {
>>>                KeyEntry ke = (KeyEntry)entry;
>>>                if (ke.chain == null || ke.chain.length == 0) {
>>>                    return null;
>>>                }
>>>                return ke.chain[0];
>>>            }
>>> 
>>> 623-629 similar comment as above
>>> 
>>> * ListKeychainStore.sh
>>> 
>>> # See JDK-8046777
>>> 
>>> 123,166: No need to add this comment, @bug label and source code history is sufficient.
>>> 
>>> 156 if [ $RECOUNT -lt `expr $COUNT + 3 + 1` ]; then
>>> 
>>> change to $COUNT + 4
>>> 
>>> 157     echo "Error: expected >=4 private key entries in the Keychain keystores"
>>> 
>>> $COUNT is the number of private keys in the keystore before testing, so original code is still correct: "expected >$COUNT"
>>> 
>>> --Sean
>>> 
>>> On 10/29/2014 07:48 PM, Vincent Ryan wrote:
>>>> Please review this fix contributed by David Kocher.
>>>> 
>>>> It corrects an issue in the Keychain keystore implementation for Mac OS where private keys
>>>> configured with a key usage other than ‘any’ are not retrieved.
>>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8046777
>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8046777/webrev.00/
>>>> 
>>>> Thanks.
>>>> 
>>