[crac] RFR: Support passing extra options in CREngine [v3]

Anton Kozlov akozlov at openjdk.org
Tue Jun 13 16:48:10 UTC 2023

On Tue, 13 Jun 2023 15:05:51 GMT, Radim Vansa <duke at openjdk.org> wrote:

>> src/hotspot/os/linux/os_linux.cpp line 440:
>>> 438: static const char* _crengine = NULL;
>>> 439: static char* _crengine_arg_str = NULL;
>>> 440: static const char* _crengine_args[32] = { NULL, NULL, NULL };
>> Should it be really 3 NULLs?
> Yes. First is in place of the binary; second is the operation (`checkpoint`/`restore`) and we want the third position initialized to NULL - when add_crengine_arg is called it should find a NULL and paste the value in there, initializing the 'tail' to NULL.

OK, thanks, make sense

>> src/hotspot/os/linux/os_linux.cpp line 5945:
>>> 5943: static void add_crengine_arg(const char *arg) {
>>> 5944:   for (size_t i = 2; i < ARRAY_SIZE(_crengine_args) - 1; ++i) {
>>> 5945:     if (_crengine_args[i] == NULL) {
>> The last empty position can be remembered along _crengine_args
> The fewer static vars the better IMO, but I can certainly use an explicit index var and drop the initialization to three NULLs if you think it's better.

In general I agree, but since _crengine_args is already stateful, I don't see any troubles adding more statics that is in fact a part of the same state. But this is minor, so up to you. OK for me as it is.


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

More information about the crac-dev mailing list