Code Review: 7126889: Incorrect SSLEngine debug output

[Brad Wetmore]

On 1/26/2012 2:00 AM, Xuelei Fan wrote:
> The fix looks fine to me except some minor comments.
>
> 1. The copyright year should be 2012 now.

Egads...

> 2. EngineArgs.java
> There is a resetPos() method in the class, which will reset application
> positions to original status. In the current fix, the "appRemaining"
> variable is not reset within the resetPos() method. As a result, after
> the call of resetPos(), the value of "appRemaining" variable may be not
> the expected value. For example,
> EngineArgs args = new EngineArgs(); // suppose the appRemaining value is 5
> args.gather(2);
> int remaining = args.getAppRemaining(); // remaining should be 3;
> args.resetPos();
> remaining = args.getAppRemaining(); // remaining is expected to be 5
> // but indeed the value is 3.
 >
> The fix is OK in that in our implementation, once we reset the position
> of an instance of EngineArgs, we will abolish the instance and never use
> it any more.

As you noticed, that is only in the error case and this EngineArgs 
instance is going away very soon, but your point is well taken.

> To avoid any miss-use of this class in the future, I would like to have
> a method comment on resetPos() to talk about the note, or more effort,
> to reserve the original app remaining value and reset appRemaining to it
> in resetPos().

We've been working together a long time.  That's a comment I would make 
if I noticed the same thing!  ;)  Thanks.

I also noticed a copy/paste error in the SSLEngineImpl exception code 
that I've cleaned up.

This is a minor change, so I'll probably just check in following JPRT.

If you're interested, the update is in the same location:

      http://cr.openjdk.java.net/~wetmore/7126889

but iteration *.01.

Brad

> Thanks,
> Xuelei
>
> On 1/26/2012 1:15 PM, Brad Wetmore wrote:
>> Xuelei (or anyone else available to review this 1-line change),
>>
>> 7126889: Incorrect SSLEngine debug output
>>
>> The JSSE debug information is currently reporting one extra byte being
>> written out, but is not actually doing that. This is just a simple
>> adjustment to correct that error.
>>
>> http://cr.openjdk.java.net/~wetmore/7126889
>>
>> Both jdk8 and jdk7u are there, the fix is identical.
>>
>> grep'ing System.out debug output in a shell script seemed to be the
>> easiest way to test. Alan/Michael, let me know if you have a better idea.
>>
>> The changes will also be pushed into 5/6/6-open in a separate effort.
>>
>> Brad
>>
>