[crac] RFR: Fix: arguments supplied to restore are split with whitespace

[YizhePKU]

On Tue, 15 Aug 2023 08:49:33 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> This PR fixes a bug that causes arguments with whitespaces to be split into multiple arguments during restore.
>> 
>> It contains two commits. The first commit is refactor only. It moves all side effects from `CracRestoreParameters` to the call sites, and changes the type of `CracRestoreParameters::args` from a single string to a `GrowableArray` of strings. Serialization code is also cleaned up a bit.
>> 
>> The second commit fixes the bug by introducing a new pseudo property, `-DCRaCJavaMainArgs`, that is set in `JavaMain`. An instance of `JavaMainArgs` (containing `argc` and `argv`) is stored as extra info of the property, which is later extracted in `Arguments::parse_options_for_restore` and passed to `crac::restore`.
>> 
>> Potential issues:
>> 
>> * We use `putenv` to modify environment variables, which expects `char*`. In this PR, I'm `const_cast`ing from `const char*` to `char*`, since I believe `putenv` doesn't actually modify the string. Is that OK? Maybe rewriting with `setenv` would be better?
>> * `read_growable_array` is implemented by reading byte-at-a-time from the shared memory. Will it be too slow? I could rewrite it to read everything at once (like the original code did), but the current implementation is cute and I kinda want to keep it.
>
> src/hotspot/share/runtime/arguments.cpp line 2287:
> 
>> 2285:         }
>> 2286:       }
>> 2287:       else if (strcmp(key, "CRaCJavaMainArgs") == 0) {
> 
> System properties usually use the dotted lowercase notation; this looks more like a VM options. I suggest something like `jdk.internal.crac.mainArgs`.

Done.

> I was also considering the `extraInfo` with a null-separated and null-null-terminated data, but individual argument can be empty so this wouldn't fly.

Actually, that might just work, as long as we pass `argc` along. I'll give it a try.

> src/hotspot/share/runtime/crac.cpp line 551:
> 
>> 549: // Write a GrowableArray to fd.
>> 550: // On error, return false.
>> 551: static bool write_growable_array(int fd, GrowableArray<const char *>* array) {
> 
> Nitpick: can the array be const?

Done.

> src/java.base/share/classes/jdk/crac/Core.java line 233:
> 
>> 231:         if (newArguments != null && newArguments.length > 0) {
>> 232:             try {
>> 233:                 Method newMain = AccessController.doPrivileged(new PrivilegedExceptionAction<Method>() {
> 
> I think that with recent changes the anonymous class could be converted back into more concise lambda, could you try?

It's a good idea, but I think it's better to leave this to another PR.

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

PR Review Comment: https://git.openjdk.org/crac/pull/101#discussion_r1297455622
PR Review Comment: https://git.openjdk.org/crac/pull/101#discussion_r1297168945
PR Review Comment: https://git.openjdk.org/crac/pull/101#discussion_r1297433473
PR Review Comment: https://git.openjdk.org/crac/pull/101#discussion_r1297439722