RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v7]

[Alan Bateman]

On Thu, 1 Jun 2023 05:20:31 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> src/hotspot/share/prims/jvmtiAgent.cpp line 512:
>> 
>>> 510: 
>>> 511:   // Print warning if EnableDynamicAgentLoading not enabled on the command line
>>> 512:   if (!FLAG_IS_CMDLINE(EnableDynamicAgentLoading) && !agent->is_instrument_lib() && !JvmtiAgentList::is_loaded(library)) {
>> 
>> The use of `!JvmtiAgentList::is_loaded(library)` here is a bit confusing. Isn't the library always already loaded by the time we get here (the assert below seems to imply that)? If so, wouldn't it already be in the list? If it's not in the list yet, perhaps a comment explaining why would be helpful here.
>
> It looks like you are right.
> There is also and assert at line 519:
> `519   assert(agent->is_loaded(), "invariant");`
> 
> So, the agent has to be loaded if we got to the line 512.
> Also, there is a statement at line 507 (before line 512):
> `507    agent->set_os_lib(library);`

> The use of `!JvmtiAgentList::is_loaded(library)` here is a bit confusing. Isn't the library always already loaded by the time we get here (the assert below seems to imply that)? 

JvmtiAgentList::load_agent creates the JvmtiAgent, attempts to load it, and adds it to the agent list if is succeeds. The test that you are looking is checking the list of already loaded agents. Maybe the function name "is_loaded" is the confusion here, maybe a better name is needed.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212610098