FYI updated webrev at: http://cr.openjdk.java.net/~vinnie/8079129/webrev.01/ <http://cr.openjdk.java.net/~vinnie/8079129/webrev.01/>
> On 5 May 2015, at 15:53, Vincent Ryan <[email protected]> wrote: > > I’ll skip the initialization. > Thanks. > > >> On 5 May 2015, at 15:52, Weijun Wang <[email protected]> wrote: >> >> That's good, but there is no need to assign null in >> >> Certificate[] certs = null; >> >> Or, maybe you can add "if (certs != null)" around the loop, but you might >> not like an extra indentation. >> >> --Max >> >> On 5/5/2015 10:44 PM, Vincent Ryan wrote: >>> OK. How about this? >>> >>> --- a/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java >>> +++ b/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java >>> @@ -1,5 +1,5 @@ >>> /* >>> - * Copyright (c) 1999, 2014, Oracle and/or its affiliates. All rights >>> reserved. >>> + * Copyright (c) 1999, 2015, Oracle and/or its affiliates. All rights >>> reserved. >>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. >>> * >>> * This code is free software; you can redistribute it and/or modify it >>> @@ -1642,23 +1642,22 @@ >>> Entry entry = entries.get(alias); >>> >>> >>> // certificate chain >>> - int chainLen = 1; >>> Certificate[] certs = null; >>> >>> >>> if (entry instanceof PrivateKeyEntry) { >>> PrivateKeyEntry keyEntry = (PrivateKeyEntry) entry; >>> - if (keyEntry.chain == null) { >>> - chainLen = 0; >>> - } else { >>> - chainLen = keyEntry.chain.length; >>> - } >>> - certs = keyEntry.chain; >>> - >>> + if (keyEntry.chain != null) { >>> + certs = keyEntry.chain; >>> + } else { >>> + certs = new Certificate[0]; >>> + } >>> } else if (entry instanceof CertEntry) { >>> - certs = new Certificate[]{((CertEntry) entry).cert}; >>> + certs = new Certificate[]{((CertEntry) entry).cert}; >>> + } else { >>> + certs = new Certificate[0]; >>> } >>> >>> >>> - for (int i = 0; i < chainLen; i++) { >>> + for (int i = 0; i < certs.length; i++) { >>> // create SafeBag of Type CertBag >>> DerOutputStream safeBag = new DerOutputStream(); >>> safeBag.putOID(CertBag_OID); >>> >>> >>> >>>> On 5 May 2015, at 15:10, Weijun Wang <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> >>>> Anyway it looks redundant and error-prone to maintain the length of an >>>> array in a separate variable. >>>> >>>> --Max >>>> >>>> On 5/5/2015 8:32 PM, Vincent Ryan wrote: >>>>> Replacing the for loop below with a for-each loop on certs would be >>>>> fine except that certs can be null. >>>>> I could initialize certs with an empty array on each iteration of the >>>>> outer loop but it doesn’t seem to gain much overall. >>>>> >>>>> >>>>>> On 4 May 2015, at 13:10, Weijun Wang <[email protected] >>>>>> <mailto:[email protected]>> wrote: >>>>>> >>>>>> 1662 for (int i = 0; i < chainLen; i++) { >>>>>> >>>>>> >>>>>> On 5/4/2015 6:08 PM, Vincent Ryan wrote: >>>>>>> Which line? >>>>>>> >>>>>>>> On 2 May 2015, at 02:22, Weijun Wang <[email protected] >>>>>>>> <mailto:[email protected]>> wrote: >>>>>>>> >>>>>>>> Is it safe to just run for-each on certs (if it's not null)? >>>>>>>> >>>>>>>> --Max >>>>>>>> >>>>>>>> On 5/2/2015 6:39 AM, Vincent Ryan wrote: >>>>>>>>> Please review this fix to correct the PKCS12 encoding when a >>>>>>>>> secret key is being stored in one keystore entry and a >>>>>>>>> certificate in another. >>>>>>>>> >>>>>>>>> Thanks. >>>>>>>>> >>>>>>>>> >>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8079129 >>>>>>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8079129/webrev.00/ >>>>>>>>> >>>>>>> >>>>> >>> >
