On Thu, 7 Mar 2024 11:57:07 GMT, Sean Coffey <coff...@openjdk.org> wrote:
>> Proposal to improve the `java.security.debug` output so that options exist
>> to add thread ID, thread name, source of log record and a timestamp
>> information to the output.
>>
>> examples:
>> format without patch :
>>
>>
>> properties: Initial security property:
>> package.definition=sun.misc.,sun.reflect.
>> properties: Initial security property: krb5.kdc.bad.policy=tryLast
>> keystore: Creating a new keystore in PKCS12 format
>>
>>
>> format with thread info included:
>>
>>
>> properties[10|main|Security.java:122]: Initial security property:
>> package.definition=sun.misc.,sun.reflect.
>> properties[10|main|Security.java:122]: Initial security property:
>> krb5.kdc.bad.policy=tryLast
>> keystore[10|main|KeyStoreDelegator.java:216]: Creating a new keystore in
>> PKCS12 format
>>
>>
>> format with thread info and timestamp:
>>
>>
>> properties[10|main|Security.java:122|2024-03-01 14:59:42.859 UTC]: Initial
>> security property: package.definition=sun.misc.,sun.reflect.
>> properties[10|main|Security.java:122|2024-03-01 14:59:42.859 UTC]: Initial
>> security property: krb5.kdc.bad.policy=tryLast
>>
>>
>> It's a similar format to what can be seen when the TLS (javax.net.debug)
>> debug logging option is in use
>>
>> current proposal is to keep the thread and timestamp information off (make
>> it opt in)
>>
>> The extra decorator info is controlled by appending option to each component
>> specified in the `"java.security.debug"` option list.
>>
>> e.g
>>
>> `-Djava.security.debug=properties+timestamp+thread` turns on logging for the
>> `properties` component and also decorates the records with timestamp and
>> thread info
>>
>> -Djava.security.debug=properties+thread+timestamp,keystore would decorate
>> the `properties` component but no decorating performed for the `keystore
>> `component.
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use default hex output
Some initial comments.
src/java.base/share/classes/sun/security/util/Debug.java line 75:
> 73: if (args.equals("help")) {
> 74: Help();
> 75: } else if (args.contains("all")) {
Suggest adding a comment explaining why you need to treat `all` specially here.
src/java.base/share/classes/sun/security/util/Debug.java line 126:
> 124: System.err.println("+thread can be appended to any of above
> options to print");
> 125: System.err.println(" thread information for that
> debug option");
> 126: System.err.println();
Would it be more reasonable to place these lines after line 113 ("x509") which
are the main options?
src/java.base/share/classes/sun/security/util/Debug.java line 181:
> 179: d.printDateTime = getConfigInfo(option, "+timestamp");
> 180: if (d.printDateTime && !dateTimeFormatInitialized) {
> 181: // trigger loading of Locale service impl now to avoid
You may want to try the test case added in JDK-8280890 with debugging enabled
to make sure you don't get a similar recursion issue.
-------------
PR Review: https://git.openjdk.org/jdk/pull/18084#pullrequestreview-1935043291
PR Review Comment: https://git.openjdk.org/jdk/pull/18084#discussion_r1523814610
PR Review Comment: https://git.openjdk.org/jdk/pull/18084#discussion_r1523811142
PR Review Comment: https://git.openjdk.org/jdk/pull/18084#discussion_r1523820467
