On Sun, 4 Jun 2023 09:38:51 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to >> allow deployment to choose whether to allow agents to be loaded/started in >> the VM. The VM option does the right thing for tools using the Attach API >> but jcmd JVMTI.agent_load was missed. This should be fixed to disallow >> loading JVMTI agents when the EnableDynamicAgentLoading is false. >> >> The CSR is: >> [JDK-8309250](https://bugs.openjdk.org/browse/JDK-8309250): jcmd >> JVMTI.agent_load should obey EnableDynamicAgentLoading >> >> Testing: >> - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java` >> - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: add to TestJcmdNoAgentLoad default and enabled dynamic loading modes Implementation change looks fine. Once your branch is sync up to main line then it should mean EnableDynamicAgentLoading is only used in one function, so easy to understand. test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java line 68: > 66: private static final String[] CMD = new String[] { > "JVMTI.agent_load", "Agent.jar" }; > 67: private static final String PTRN = "Dynamic agent loading is not > enabled"; > 68: private static boolean enableDynLoad = true; It might be clear to change this to be a static final field name "dynamicLoadingEnabled", just suggesting "enabled" rather than "enable" as the usage in this test is to see if the option is enabled. ------------- Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14244#pullrequestreview-1461132592 PR Review Comment: https://git.openjdk.org/jdk/pull/14244#discussion_r1216591017
