On Thu, 28 Aug 2025 23:54:42 GMT, Thomas Fitzsimmons <[email protected]> wrote:
>> Perhaps check the existence of the file and error out with the config file
>> and its path if the check fails, this way, it's crystal clear.
>
> Thank you for reviewing.
>
>> Hmm, I find it somewhat obscure that the config variant property changes the
>> value of the config file name.
>
> Yes, I see your point.
>
>> With this new config variant property, it assumes that the confg file name
>> has a "." which is probably true most if not all times.
>
> The regular expression supports appending to a file without a ".":
>
>
> $ jshell -q
> jshell> "kryoptic".replaceFirst("(\.[^\.]*)?$", "-" + "sensitive" + "$1");
> $1 ==> "kryoptic-sensitive"
>
>
> I should have added this case to the comment you mentioned above, will do in
> the expanded comment you requested.
>
>> We should document all these properties so it's clear their precedence as
>> well as the assumptions/implications. All these security can be set
>> independently, right? It's a bit strange that you set the CUSTOM_P11_CONFIG
>> NAME and then setting the config variant property would actually changes the
>> config file to a different name.
>
> Yes, conceptually I am treating file pairs like `p11-nss.txt` and
> `p11-nss-sensitive.txt` (and `p11-kryoptic.txt`,
> `p11-kryoptic-sensitive.txt`) as variants of the same configuration. When
> `CUSTOM_P11_CONFIG_VARIANT` is set, `CUSTOM_P11_CONFIG_NAME`'s meaning
> becomes something like "base configuration file name".
>
> Given the current test suite, and how I am specifying the use of Kryoptic, I
> wouldn't expect both `CUSTOM_P11_CONFIG_VARIANT` and `CUSTOM_P11_CONFIG_NAME`
> (or `CUSTOM_P11_CONFIG`) to be specified by the user at the same time.
> `CUSTOM_P11_CONFIG_VARIANT` is meant for hard-coding in tests that invoke the
> test VM separately in sensitive and normal modes.
>
>> Perhaps check the existence of the file and error out with the config file
>> and its path if the check fails, this way, it's crystal clear.
>
> OK, I can do that. I will add a `/** ... */` block above `getNssConfig`.
> These two changes will hopefully reduce the weirdness of the
> `CUSTOM_P11_CONFIG_NAME`/`CUSTOM_P11_CONFIG_VARIANT` combination. I will
> also document the existing `CUSTOM_P11_CONFIG_NAME` versus
> `CUSTOM_P11_CONFIG` precedence since that might also be surprising when both
> are set.
Done. See what you think. I am still open to other options. One idea I
played around with was adding a `CONFIG_P11_CONFIG_BASE_NAME` that is just part
of the file name, say `p11-nss` by default. Then in `getNssConfig` the file
name could be built from the base and variant strings, instead of by changing
the name. Conceptually that might be clearer but I think it would be a more
invasive change given that the existing code deals with file names. And,
having experimented with it, I think the new exception you suggested will make
it fairly obvious what to fix if someone hits a missing configuration file.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26325#discussion_r2311670763
