On Fri, 22 Nov 2024 19:32:32 GMT, Bradford Wetmore <wetm...@openjdk.org> wrote:
>> Sean Coffey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> enum Options and logic
>
> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 159:
>
>> 157: }
>> 158:
>> 159: if (checkPoints.equals("ssl")) {
>
> I always thought that the `ssl` options were additive.
>
> The presence of `ssl` in the property outputs those debug statements which
> only have `ssl` defined. The presence of additional options turns on
> additional debug info. i.e. The property `ssl,handshake` turns on both `ssl`
> and `ssl,handshake` statements. `ssl,handshake,verbose` turns on `ssl`,
> `ssl,handshake`, and `ssl,handshake,verbose` statements.
>
> I **think** this style would eliminate these special cases, and the code
> would just go to the `split`/options loop after the EMPTYALL check.
No, the `ssl `option is designed to return all debug statements except those
that are extra verbose (`data`, `packet`, `plaintext`). At least that's how the
old (JDK 7) and newer TLSv1.3 stacks have been implemented. (with the exception
of bug 8044609 which is being addressed here!)
The `record`, `handshake`, `keygen`, `session`, `defaultctx`, `sslctx`,
`keymanager`, `trustmanager ` options have always struck me as odd. I reckon no
one uses them. The only logic I can see with them is as filtering operations,
since the `all `or `ssl `option is mandatory.
I think the filtering option (if we choose to keep them) is the only purpose we
can have for these sub options.
I can't imagine telling users to use `ssl` for some TLS debugging but turn on
`record` or `handshake` or `keygen` or `session` or `defaultctx` or `sslctx` or
`keymanager` or `trustmanager` options if you fancy other bits of TLS debugging.
Happy to address your CSR comments shortly. Let's agree on the implementation
some more first. Nice suggestion on that EMPTYALL value. I'll get that change
in shortly.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1856566187
