RFR: JDK-8032573

[Jamil Nimeh]

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.nimeh at 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
>>