On 10 Nov 2014, at 18:13, Sean Mullan <sean.mul...@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.mul...@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. >>>> >>
