On Fri, 30 Jan 2026 23:36:46 GMT, Bradford Wetmore <[email protected]> wrote:
>> Sean Coffey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review comments from Brad
>
> src/java.base/share/classes/sun/security/ssl/CertificateMessage.java line
> 1026:
>
>> 1024: ka, hc.negotiatedProtocol) != null
>> 1025: ||
>> SSLLogger.logWarning(SSLLogger.Opt.HANDSHAKE,
>> 1026: "Unable to produce
>> CertificateVerify for key algorithm: " + ka))
>
> Nitty nit nit: might as well take care of the <80 here too. Esp line1021.
> ;)
>
> Had to vertically scroll just to see the line in side-by-side. Thanks!
>
> There were a few others where things got a bit long (e.g.
> `SessionTicketExtension`, `SSLCipher`), and noticed a couple really egregious
> existing ones. I'll probably take care of them when I get to the category
> bug.
>
> (P.S. Thank you for doing that in `CertificateRequest.java`!)
Sure - cleaned up around 1021. A lot of the wide line code is already
pre-existing. might be no harm to fix up in follow on.
> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 239:
>
>> 237: " all non-widening filters are enabled.%n%n");
>> 238: System.err.printf("%nAdding valid filter options to \"ssl\"
>> will log" +
>> 239: " messages to include%njust those filtered
>> categories.%n");
>
> So the intended behavior is the following:
>
> If we have code like this:
>
>> if (SSLLogger.isOn() && SSLLogger.isOn(SSLLogger.Opt.SSL)) {
>
> then setting `javax.net.debug=ssl` or `ssl,trustmanager`, it will always
> output this logging statement.
>
> But if we have code like this:
>
>> if (SSLLogger.isOn() && SSLLogger.isOn(SSLLogger.Opt.TRUSTMANAGER)) {
>
> then `javax.net.debug=ssl` will output this (because only `ssl` was enabled),
> but `javax.net.debug=ssl,keymanager` will not. That's because `keymanager`
> is a valid name, thus only the `SSL` and `KEYMANAGER` options will print).
>
> So we need to make sure that everything `SSL` and non-`SSL` is categorized
> correctly, which will be the focus of
> [JDK-8344158](https://bugs.openjdk.org/browse/JDK-8344158).
>
> The wording here needs a slight update, and I'm not quite sure how to word
> this. Ideas? :)
>
> Adding valid filter options to ssl will output the high-level SSL/TLS
> debug
> messages, plus only those messages in the specified categories.
yes, that's the logic in use now. The sub-component options need to be regarded
as filters.
regards the wording, it's hard to chose the best description. What's there
accurate in some respect.
How about:
`Specifying filter options with "ssl" includes messages for the selected
categories, as well as all general SSL debug messages.`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2764744810
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2764743889
