On 5/28/2019 3:14 PM, Martin Balao wrote:
If we could check the transformation directly, it might be better to
have the check in the SSLCipher constructors, using immutable enum field.
Putting them together:
SSLCipher.java:
private SSLCipher(String transformation,
...
this.isAvailable =
- allowed && isUnlimited(keySize, transformation);
+ allowed && isUnlimited(keySize, transformation) &&
+ isAvailable(transformation);
}
+ private static boolean isAvailable(String transformation) {
+ if (!transformation.equals("NULL")) {
+ ... // check the instance
+ } else {
+ ...
+ }
+ }
SSLContextImpl.java:
382 if (!suite.supports(protocol) ||
-383 !suite.bulkCipher.supports(protocol)) {
+383 !suite.isAvailable()) {
* I guess you meant suite.bulkCipher.isAvailable().
No, I meant suite.isAvailable(), but suite.bulkCipher.isAvailable() also
works for the update. I will leave it to you for the final decision,
not more code review required to me.
Yes, this was something I considered at the beginning but had some
design concerns. The Read/WriteCipherGenerator is chosen according to
the protocol, and is the one that may have problems with the
transformation when creating an instance. This generic design gives me
the idea that there may be different cipher implementations (per
protocol) and even per write or read operation. We can simplify based on
the current implementation but, at some point, I believe we are going
against the generic design. Anyways, I'll take it.
With "!suite.supports(protocol) || !suite.isAvailable()", the protocol
specific is checked with suite.supports(protocol), while the
availability is checked with suite.isAvailable(). I think the design
should be fine.
It's still possible to have the "available transformations" cache but I
guess, per your first comment, that you prefer not to have it.
I'm about 50% to 50% to use cache in enum constructor: a balance of
memory or CPU. Once the enum get constructed, the cache is useless. So
I prefer your current update unless the duplicated transformations is
not a few.
Here it's Webrev.03 with all the previous changes:
* http://cr.openjdk.java.net/~mbalao/webrevs/8223482/8223482.webrev.03/
It looks good to me.
Thank you very much for taking my comments!
Regards,
Xuelei
