Good.

Thanks
Max

On 5/5/2015 11:17 PM, Vincent Ryan wrote:
FYI updated webrev at:
http://cr.openjdk.java.net/~vinnie/8079129/webrev.01/


On 5 May 2015, at 15:53, Vincent Ryan <[email protected]
<mailto:[email protected]>> wrote:

I’ll skip the initialization.
Thanks.


On 5 May 2015, at 15:52, Weijun Wang <[email protected]
<mailto:[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]>
<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]>
<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]>
<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/






Reply via email to