On Fri, 16 Apr 2021 11:24:57 GMT, Sean Coffey <coff...@openjdk.org> wrote:
> Added capability to allow the PKCS11 Token to be destroyed once a session is
> logged out from. New configuration properties via pkcs11 config file. Cleaned
> up the native resource poller also.
>
> New unit test case to test behaviour. Some PKCS11 tests refactored to allow
> pkcs11 provider to be configured (and tested) with a config file of choice.
>
> Reviewer request @valeriepeng
Here are some comments. Mostly just nit. Will continue looking at the test
changes.
Thanks~
Valerie
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java line 434:
> 432: throw excLine(word + " must be at least 100 ms");
> 433: }
> 434: } else if (word.endsWith("cleaner.shortInterval")) {
Why use endsWith()? Most of other parsing code are enforcing equality, i.e.
equals()?
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/KeyCache.java line 2:
> 1: /*
> 2: * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights
> reserved.
2021? Same goes for other files.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java line 176:
> 174: found = true;
> 175: next.dispose();
> 176: }
nit: maybe this can be shortened a little with:
SessionKeyRef next;
while ((next = (SessionKeyRef) SessionKeyRef.refQueue.poll()) != null) {
found = true;
next.dispose();
}
Same goes for the other drainRefQueue() impl.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Session.java line 133:
> 131: }
> 132:
> 133: static boolean drainRefQueue() {
Add a comment on this being called by the cleaner thread with the two interval
configuration options? Sames goes for the one in P11Key.java.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Session.java line 152:
> 150: implements Comparable<SessionRef> {
> 151:
> 152: static ReferenceQueue<Session> refQueue =
nit: can now move the value assignment onto the same line. Same goes for the
refQueue in P11Key.java.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SessionManager.java
line 246:
> 244: private final AbstractQueue<Session> pool;
> 245: private final int SESSION_MAX = 5;
> 246: // access is synchronized on 'this'
Maybe old comment?
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line
963:
> 961: private long sleepMillis =
> config.getResourceCleanerShortInterval();
> 962: private int count = 0;
> 963: boolean p11RefFound, SessRefFound;
nit: p11RefFound => keyRefFound?
nit: SessRefFound => sessRefFound?
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line
992:
> 990: // increase the sleep time
> 991: sleepMillis =
> config.getResourceCleanerLongInterval();
> 992: }
This if-check can be moved up to below the line "count++;"?
-------------
PR: https://git.openjdk.java.net/jdk/pull/3544
