[crac] RFR: Support passing extra options in CREngine

[Radim Vansa]

On Thu, 8 Jun 2023 18:38:10 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> In addition to `-XX:CREngine=program` this adds support to `-XX:CREngine=program,key=value,anotherkey` that translates into invoking `program --key value --anotherkey`.
>> 
>> This generic parameters support is utilized in `criuengine` that accepts `--verbosity` and `--log-file` options and relays them to `criu`.
>
> src/hotspot/share/runtime/globals.hpp line 2100:
> 
>> 2098:       "as a comma-separated list of key[=value] pairs; "                    \
>> 2099:       "-XX:CREngine=program,key=value,anotherkey results in calling "       \
>> 2100:       "'program --key value --anotherkey'")                                 \
> 
> There is CRAC_CRIU_OPTS that is used to pass additional options to CRIU. I see that remains in place, and this turns out to be alternative?
> 
> Not every CREngine may want to follow this convention of argument passing. Usually, `program,--key,value,--anotherkey` is specified to call `program --key value --anotherkey`.
> 
> This probably needs to be RESTORE_SETTABLE.

Ouch, I admit that I've missed `CRAC_CRIU_OPTS` (haven't seen it documented anywhere). I would keep it for backwards compatibility, but having options that are explicitly parsed and handed-over by criuengine seems more reliable. As in the case of the verbosity & log file: I wanted to override the default `-v1`.

> Not every CREngine may want to follow this convention of argument passing. Usually, program,--key,value,--anotherkey is specified to call program --key value --anotherkey.

I have selected the `key=value` syntax rather than simply concatenating args as that is used e.g. by FlightRecorder, and IMO is looks better when passing as VM options. Yes, it might by a hypocrisy since this is just passing those arguments.

We should also consider the dynamic library CREngine in #78 - in the draft I am passing the arguments parsed (and turned into `--key` form), but there might be better ways.

I don't think it's really up to CREngine to decide, JVM is ruling the API and CREngine is here to adapt JVM expectations to the implementation API. CREngine has a second-class role in here.

> This probably needs to be RESTORE_SETTABLE.

Yes, this predates the flag, I haven't merged last changes until first review.

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

PR Review Comment: https://git.openjdk.org/crac/pull/63#discussion_r1224048712