Hi Xuelei, I'm working on your comments now. Thanks for all the useful
feedback. I'm working on the comments in your other emails, too.
On 06/30/2015 02:46 AM, Xuelei Fan wrote:
src/java.base/share/classes/sun/security/ssl/HandshakeMessage.java
==================================================================
676 private List<byte[]> encodedResponses = new ArrayList<>(4);
4 may be not the best estimate. Maybe better to make the initialization
in the constructor.
Fair enough. I've moved it into the constructors. It's based on the
chain length in the version where the chain is provided, and it uses the
default constructor in the case where you're bringing it in from a
HandshakeInStream (since you don't know the number of objects up-front).
---------------
line 713/714, 730/731 throws SSLHandshakeException for extension
constructor in server side. That's unlikely to happen, I think. I was
wondering, if CertificateStatus cannot be constructed, the server may
not want to send the message, rather than terminate the connection
immediately.
I think you're right. While the exception is unlikely, I'd like to have
the HandshakeMessage throw the exception if something bad happens. I do
however, agree that we shouldn't make it a fatal error. I'll catch the
exception in ServerHandshaker, log it, and just not send the message as
you suggested since that is legal. OK?
-------------
674 private int encodedResponseLen;
s/encodedResponseLen/encodedResponsesLen
Suggest to add
--------------
820 if (respBytes != null) {
821 s.putBytes24(respBytes);
822 } else {
823 s.putBytes24(new byte[0]);
824 }
HandshakeOutStream.putBytes24() accepts null parameter.
Yeah, that is a better way to do it and not have the unnecessary object
creation.
-------------
line 827/828, unlikely to happen. I would suggest you add a comment or
remove the lines.
I'll comment it.
Xuelei
On 6/27/2015 11:06 PM, Jamil Nimeh wrote:
Hello all, I've posted an updated webrev based on comments I've received
so far:
http://cr.openjdk.java.net/~jnimeh/reviews/8046321/webrev.1
Thanks,
--Jamil
On 06/18/2015 05:27 PM, Jamil Nimeh wrote:
Hello all,
I have a first cut at the OCSP stapling webrev posted for your review:
JEP: https://bugs.openjdk.java.net/browse/JDK-8046321
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8046321/webrev.0/
A couple items to note:
* I'm in the process of updating the JEP with some more details. I
should be done with these changes by tonight (PDT).
* Missing are some of the TLS end-to-end tests. These tests have
been coded and run outside the jtreg framework, but for some
reason things hang in jtreg. I've included some of the supporting
classes that these tests will use (CertificateBuilder.java and
SimpleOCSPResponder.java) so folks could review those if they're
interested. I will update the webrev and notify the list as soon
as I've got the tests working in jtreg.
Thanks to everyone who has helped along the way.
--Jamil
