Code Review: 7126889: Incorrect SSLEngine debug output

[Xuelei Fan]

looks fine to me.

Thanks,
Xuelei

Sent from my iPad

On Jan 27, 2012, at 6:58 AM, Brad Wetmore <bradford.wetmore at oracle.com> wrote:

> 
> 
> 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
>>> 
>>