On Wed, 20 Aug 2025 02:13:17 GMT, Valerie Peng <[email protected]> wrote:
>> test/jdk/sun/security/pkcs11/PKCS11Test.java line 490:
>>
>>> 488: configFilePath = configFilePath.replaceFirst(
>>> 489: "(\\.[^\\.]*)?$", "-" + customConfigVariant +
>>> "$1");
>>> 490: }
>>
>> Hmm, I find it somewhat obscure that the config variant property changes the
>> value of the config file name. With this new config variant property, it
>> assumes that the confg file name has a "." which is probably true most if
>> not all times. 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.
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26325#discussion_r2308810783
