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");
}
Easily fixed, thanks!
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)) {
I'll go back and take a crack at refactoring this and see if we can make
it a bit more elegant. I had done something that used parseExtensions
once initially using a loop but found that I mistakenly accepted a case
where the two context-specific tags were both present, but reversed in
their order. Regardless, I'll take a whack at it and see what happens.
Thanks for the comments!
--Jamil
--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
