On 03/13/2015 08:43 AM, Sean Mullan wrote:
On 03/11/2015 06:10 PM, Jamil Nimeh wrote:
Okay, an updated webrev has been posted that addresses Sean's comments
(thanks, BTW).
http://cr.openjdk.java.net/~jnimeh/reviews/8074064/webrev.04/
Code changes look good, but I had not reviewed the test so here are a
few more comments on that part.
- for the path, can you remove "Tests" from the directory name?
Everything is a test so that seems redundant to me. So the new path
would be test/sun/security/provider/certpath/OCSP
Yeah, I guess it is redundant, isn't it? I can change that. I probably
should do the same with a bunch of my OCSP stapling unit tests too.
- Why do you need the DEBUG boolean property? When would you ever want
to turn this off? Most of the debug info is valuable (like "Single
Extension count mismatch ...") so when something fails you would
always want to see it in the logs.
Force of habit. Some of my other tests, particularly in the SSL arena,
have debug output that can get very chatty, above and beyond what would
be useful for normal log output. For a test like this though, what you
say makes sense. I'll rip that out and just go with normal
System.out.println output.
- readFile should create the FileReader in a try-with-resources block
so it is closed.
Oh wow. I should've caught that.
- line 165 needs a 4-space indent
All these will be fixed up in a jiffy.
--Sean
--Jamil
On 03/11/2015 07:38 AM, Sean Mullan wrote:
Hi Jamil,
Just a few comments, mostly minor.
* OCSPResponse
768: there is an extra space in the indentation
811: use Collections.emptyMap instead of new HashMap<>() as that does
not allocate a new Map object.
813-15,861-63: Use Map.values instead of Map.keySet, ex:
for (Extension ext : singleExtensions.values()) {
sb.append("singleExtension: " + ext + "\n");
}
784-807: I think this code could be compressed a bit to avoid calling
parseExtensions twice. You can eliminate lines 786-92, if you do
something like this (I think), starting at line 784:
if (tmp.available() > 0) {
derVal = tmp.getDerValue();
} else {
derVal == null;
}
then what is now line 794 becomes:
if (derVal != null && derVal.isContextSpecific((byte)1)) {
--Sean
On 03/04/2015 05:50 PM, Jamil Nimeh wrote:
One more round of updates. Only the test code and the test inputs
have
changed. Test input is now Base64 format, with comment headers that
display the OCSP response pretty-print (or an asn1parse of the
BasicOCSPResponse for malformed response test cases).
http://cr.openjdk.java.net/~jnimeh/reviews/8074064/webrev.03/index.html
Thanks, Vinnie for the feedback and suggestions!
--Jamil
On 03/03/2015 04:10 PM, Jamil Nimeh wrote:
Okay, I've got an updated webrev for this issue:
http://cr.openjdk.java.net/~jnimeh/reviews/8074064/webrev.02/index.html
Thanks,
--Jamil
On 03/03/2015 02:18 PM, Jamil Nimeh wrote:
Hello all, I've come across a couple edge cases that this fix
doesn't
cover properly. I'll put out another webrev in a bit that should
tighten up the singleResponse parsing, particularly with the
optional
fields. It will also include a couple other negative test input
samples.
Thanks,
--Jamil
On 03/02/2015 04:05 PM, Jamil Nimeh wrote:
Hello all, this review fixes an issue in OCSPResponse where it does
not parse singleExtensions in the SingleResponse structure. I also
fixed two minor deviations from the OCSP.RevocationStatus interface
when getRevocationTime and getRevocationReason are called on good
responses.
Bug: https://bugs.openjdk.java.net/browse/JDK-8074064
Review:
http://cr.openjdk.java.net/~jnimeh/reviews/8074064/webrev.01/index.html
Thank you,
--Jamil
