[crac] RFR: Allow users to pass new properties on restore

[Ashutosh Mehra]

On Wed, 11 May 2022 18:27:59 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>>> I think we need to choose one model (JVMTI and j.l.S.properties are identified for now), follow it, and try to understand how much it satisfies users' needs. Or at least ours :)
>> 
>> From this perspective I think we can go with not updating `_system_properties` on restore. Right now the requirement, as I understand, is to provide a way to specify new or update existing application specific properties on restore which can be achieved by just updating the properties in `j.l.System`.
>> 
>>> I would like to distinguish setting a value for a property and the way or the point when the value is processed by some unrelated entity to the way the property is set. Handling is a very implementation detail.
>> 
>> I get your point. Let's drop the "modifiable" marker on the properties and just update the `java.lang.System.getProperties` view so that applications can make use of the new/updated properties.
>> 
>>> Having that there is another pass over all of the arguments anyway, is it possible to provide all explicit native arguments to the function doing the restore (os::Linux::restore for now)? Then we won't need to introduce a lot of aux code in the VM and it will be simpler to move out VM eventually.
>> 
>> Did you mean we just pass the properties and application arguments directly to `os::Linux::restore()` instead of getting it through `Arguments` class? It should be possible. I guess as soon as we get access to the args, we can call `os::Linux::restore()` and avoid all the JVM initialization process.
>> The `args` parameter to `Threads::create_vm()` has all the stuff that we need to pass to `os::Linux::restore()`. We can do a quick pass over `args` to extract the properties and application arguments. 
>> The java launcher adds following properties to the `args` in addition to the user specified:
>> 1. -Djava.class.path
>> 2. -Dsun.java.command
>> 3. -Dsun.java.launcher
>> 
>> We can ignore 1 and 3, collect the application arguments from 2, filter options in `args` starting with `-D` and pass the set to `os::Linux::restore()`. Is that what you are suggesting?
>
>> Right now the requirement, as I understand, is to provide a way to specify new or update existing application specific properties on restore which can be achieved by just updating the properties in j.l.System.
> 
> Seems so, although not something I would call a strict requirement -- something that has emerged as useful and clear.
> 
>> I guess as soon as we get access to the args, we can call `os::Linux::restore()` and avoid all the JVM initialization process.
> 
> Yeah, `os::Linux::restore(char** args)` for example, with -D and arguments.
> 
> At the moment we need a few -XX: arguments parsed, CRaCRestoreFrom, CREngine, and probably more. It was convenient to assume all -XX arguments are parsed and available in os::Linux::restore. But since this won't last forever, let's see how and if to preserve the existing arguments parsing. I would avoid doing big changes, but up to you. 
> 
>> It The `args` parameter to `Threads::create_vm()` has all the stuff that we need to pass to `os::Linux::restore()`. We can do a quick pass over `args` to extract the properties and application arguments. The java launcher adds following properties to the `args` in addition to the user specified:
>>
>> 1. -Djava.class.path
>> 2. -Dsun.java.command
>> 3. -Dsun.java.launcher
>>
>> We can ignore 1 and 3, collect the application arguments from 2, filter options in `args` starting with `-D` and pass the set to `os::Linux::restore()`. Is that what you are suggesting?
> 
> Apparently yes. Although if possible, it would be better to collect positional arguments directly from the arguments set instead of sun.java.command, so arguments with blank chars will be handled correctly -- this is a problem for now.
> 
> Thanks!

@AntonKozlov

> Apparently yes. Although if possible, it would be better to collect positional arguments directly from the arguments set instead of sun.java.command, so arguments with blank chars will be handled correctly -- this is a problem for now.

Even in normal mode the positional arguments are gathered from `sun.java.command`. They don't seem to be available directly from the arguments. So I have stayed with that approach.

I have updated the change set based on previous comments. Please review.

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

PR: https://git.openjdk.java.net/crac/pull/21