Looks fine to me. Thanks! Xuelei
On 3/11/2016 4:39 PM, Jamil Nimeh wrote: > Hello all, > > This updated webrev switches from holding the stapling parameters as > instance fields to local variables to the clientHello() routine where > they are used. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8132942 > Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8132942/webrev.02/ > > --Jamil > > > On 03/04/2016 02:30 AM, Xuelei Fan wrote: >> JDK-8132942: >> >> "The current implementation for OCSP stapling has ServerHandshaker >> trying to construct a CertificateStatus message, but if the arguments >> are invalid it throws SSLHandshakeException." >> >> In your webrev, looks like the exception get ignore before your update. >> I may miss something. Can you have more details about this point. >> >> >> On 3/3/2016 12:48 AM, Jamil Nimeh wrote: >>> Hello all, this fixes a minor issue with OCSP stapling, where we now do >>> the argument checking up-front before attempting to instantiate the >>> CertificateStatus handshake message object. >> I may miss something. I did not find the update related to this point. >> Can you have more details? >> >>> Also I've pulled out the >>> OCSP stapling processing from within the clientHello method since it >>> already was really long and placed it in its own private method. >>> >> The price is there are three more new class variables. I would try to >> avoid it. Looks like "staplingActive" should not be a class variable, >> too. >> >> Xuelei >> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8132942 >>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8132942/webrev.01/ >>> >>> Thanks, >>> --Jamil >
