502: sigh, yes. It needs to be pbis. I thought I caught all those.
Thanks for catching that, I'll get that fixed up.
I had considered a similar alternate approach to using a
PushbackInputStream, but I thought passing the first byte in and
checking there seemed awkward (as you said as well). I wanted to
preserve the stateless nature of readOneBlock as well, so doing the
initial single byte read and preserving its value in the caller to rOB
made sense. The two methods that return collections right now can't
distinguish if a null value on the first block is because the stream was
already at the end, or if it read some data and couldn't find anything.
That distinction matters though on the first block only. Yet in single
valued methods like generateCertificate (singlular), a null return value
from rOB value is always an exception case.
That's why I went looking for an interface that would let me consume the
stream for one byte, and then "put it back" and keep the logic of rOB
intact. PushbackInputStream doesn't seem to be too bad on performance,
since any bytes you choose to unread go onto a byte array created at
construction time (only one byte in this particular case). Subsequent
reads will pull from this byte array first and when exhausted will then
go to the backing input stream.
Thanks for the feedback,
--Jamil
On 09/29/2014 05:39 PM, Wang Weijun wrote:
X509Factory.java:
502 data = readOneBlock(is);
Should it be pbis?
Actually I would suggest reusing the variable name "is" to prevent any such
error.
Also, I am not sure if using a PushbackInputStream will hurt the performance.
The readOneBlock() method already includes the read-first-byte logic inside so
maybe we can change it a little to cover the fix. For example, I can think of
renaming it to readOneBlock(firstByte, is) so inside your fix you can call
readOneBlock(perkByte, is) and in other cases call readOneBlock(is.read(), is).
This might look a little strange but hopefully you can find a more concise one.
Thanks
Max
On Sep 30, 2014, at 5:11, Jamil Nimeh <jamil.j.ni...@oracle.com> wrote:
Hello all,
This review fixes a small regression in the generateCertificates() and
generateCRLs() methods for the CertificateFactory class. At some point, input
consisting entirely of non-certificate data ceased to throw
CertificateException or CRLException and instead returned an empty collection.
This restores the exception-throwing behavior, but only when the entire stream
is non-cert data. Cases where there is leading/trailing text around a valid
PEM-encoded certificate or CRL will still ignore the leading/trailing data and
parse the certificate/CRL properly as before.
Bug: https://bugs.openjdk.java.net/browse/JDK-8032573
Review: http://cr.openjdk.java.net/~ascarpino/8032573/webrev.01/
Thank you,
--Jamil
