On Wed, 1 Jul 2026 18:18:56 GMT, Kieran Farrell <[email protected]> wrote:
> This patch adds a new jcmd diagnostic command, `VM.show_settings`, to make > the existing -`XshowSettings` output available from a running VM. The command > accepts the same sections as`XshowSettings` (all, vm, properties, locale, > security, system, security:all, security:properties, security:providers, > security:tls). > > HotSpot registers the new diagnostic command, validates the requested > section, then calls into `sun.launcher.LauncherHelper` to reuse the existing > show settings formatting and return the result as bytes for the jcmd stream. > The VM settings path also passes hotspots `InitialHeapSize`, `MaxHeapSize`, > and Java thread stack size into the Java helper. A new jcmd test covers VM, > properties, security TLS, and invalid-input output. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). I've taken a first pass through this and it seems a reasonable thing to do. A few comments below. src/hotspot/share/services/diagnosticCommand.cpp line 510: > 508: CLEAR_PENDING_EXCEPTION; > 509: return; > 510: } AFAICS the `LauncherHelper` class is guaranteed to be loaded and initialized as part of running the `jcmd` tool. ?? src/hotspot/share/services/diagnosticCommand.cpp line 516: > 514: JavaCallArguments args; > 515: > 516: Handle option_str = java_lang_String::create_from_str(flag_buf, CHECK); Shouldn't this get the same treatment as class initialization exceptions above? src/hotspot/share/services/diagnosticCommand.cpp line 542: > 540: > 541: assert(res->is_typeArray(), "LauncherHelper.showSettingsBytes must > return byte[]"); > 542: assert(TypeArrayKlass::cast(res->klass())->element_type() == T_BYTE, > "must be byte[]"); Not sure what you are trying to guard against here, the method signature says it returns a `byte[]`. src/hotspot/share/services/diagnosticCommand.hpp line 870: > 868: > 869: static int num_arguments() { return 1; } > 870: static const char* name() { return "VM.show_settings"; } Suggestion: static int num_arguments() { return 1; } static const char* name() { return "VM.show_settings"; } src/hotspot/share/services/diagnosticCommand.hpp line 875: > 873: "'VM.show_settings locale' or 'VM.show_settings > security:tls'."; > 874: } > 875: static const char* impact() { return "Low"; } Suggestion: static const char* impact() { return "Low"; } src/java.base/share/classes/sun/launcher/LauncherHelper.java line 189: > 187: > 188: public static byte[] showSettingsBytes(String optionFlag, > 189: long initialHeapSize, long maxHeapSize, long stackSize) { Suggestion: public static byte[] showSettingsBytes(String optionFlag, long initialHeapSize, long maxHeapSize, long stackSize) { test/jdk/sun/tools/jcmd/TestJcmdShowSettings.java line 42: > 40: testPropertySettings(); > 41: testSecurityTlsSettings(); > 42: testInvalidSection(); Shouldn't there be a test case for each valid selection? ------------- PR Review: https://git.openjdk.org/jdk/pull/31742#pullrequestreview-4615035987 PR Review Comment: https://git.openjdk.org/jdk/pull/31742#discussion_r3511165591 PR Review Comment: https://git.openjdk.org/jdk/pull/31742#discussion_r3510942607 PR Review Comment: https://git.openjdk.org/jdk/pull/31742#discussion_r3510950141 PR Review Comment: https://git.openjdk.org/jdk/pull/31742#discussion_r3510959120 PR Review Comment: https://git.openjdk.org/jdk/pull/31742#discussion_r3510960793 PR Review Comment: https://git.openjdk.org/jdk/pull/31742#discussion_r3510970496 PR Review Comment: https://git.openjdk.org/jdk/pull/31742#discussion_r3510983530
