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
