Updated webrev looks fine~
Thanks,
Valerie
On 4/1/2016 10:11 AM, Anthony Scarpino wrote:
I updated the webrev:
http://cr.openjdk.java.net/~ascarpino/8098580/webrev.01/
Comments below....
On 03/29/2016 11:08 AM, Valerie Peng wrote:
Hi, Tony,
<SessionManager.java>
- line 243, 'be' should be "by"?
Yes, by.
- line 271, why use SESSION_MAX - 1 instead of just SESSION_MAX? The
check on line 286 make sure that the queue will not be empty. Actually,
the comment has "until only one is left", but the condition is ">1", so
shouldn't it be "until 2 left"? This is not what u changed, just noticed
it and wonder if I read the code wrong.
Yes, I believe you are correct, the -1 should be removed
<P11Key.java>
- line 1114 and 1115, can we rename these 2 variables? The terms session
and token are all over this SessionKeyRef class. Some are local, some
are member fields. It'd make the code much more easier to follow if they
are named something less common, say "sess" or "tok".
Sure.
- line 1126 - 1127, it's clearer to have {} to clearly separate the
code.
Oops.. Didn't notice it was missing..
- After the while-true loop (line 1116 - 1134), don't you have to call
token.releaseSession(session) again? Otherwise, the most recently
allocated op session seems to not released back to the pool.
Yes, you are right.
- There was a session.token.isValid() check in the older dispose()
method but not in the new one. Shouldn't we still check before getting a
new op session?
I put the check back in so that it will avoid doing a pkcs11
operations if the token isn't available.
- The two dispose methods do entirely different things, it'd be better
to name them differently.
Ok.. I changed the one that takes the session to disposeNative.
Thanks,
Valerie
On 3/1/2016 2:29 PM, Anthony Scarpino wrote:
Hi,
I need a review of this change to SunPKCS11 where it's a bit smarter
in disposing of native library object by reusing the same pkcs11
session is possible. Additionally changes to the session pool to fix
a performance bottleneck.
http://cr.openjdk.java.net/~ascarpino/8098580/webrev/
thanks
Tony
