looks fine to me. Thanks, Xuelei
Sent from my iPad On Jan 27, 2012, at 6:58 AM, Brad Wetmore <[email protected]> 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 >>> >>
