On Wed, 21 Aug 2024 20:21:47 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> addresses delayed provider selection where parameters are involved
>
> test/jdk/com/sun/crypto/provider/KDF/Functions.java line 36:
>
>> 34: import java.util.HexFormat;
>> 35:
>> 36: public class Functions {
>
> Since this is strictly HKDF, it'd be clearer for the test to have HKDF in its
> name, e.g. "HKDFFunctions.java" or "BasicHKDF,java".
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/9f050b6a1a4a83d8623e206323071c2c77c90bb2.
Please indicate if resolved.
> test/jdk/com/sun/crypto/provider/KDF/TestHKDF.java line 55:
>
>> 53: testName = Objects.requireNonNull(name);
>> 54: algName = Objects.requireNonNull(algStr);
>> 55: IKM = hex2bin(Objects.requireNonNull(ikmStr));
>
> why is "IKM" all uppercases? "ikm" matches the naming convention of other
> fields better.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/9f050b6a1a4a83d8623e206323071c2c77c90bb2.
Please indicate if resolved.
> test/jdk/com/sun/crypto/provider/KDF/TestHKDF.java line 241:
>
>> 239: System.out.println("\t* Key output: FAIL");
>> 240: System.out.println("Expected:\n" +
>> 241: dumpHexBytes(outData, 16, "\n", " "));
>
> `outData `should be `expectedOut`.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/9f050b6a1a4a83d8623e206323071c2c77c90bb2.
Please indicate if resolved.
> test/jdk/com/sun/crypto/provider/KDF/TestHKDF.java line 302:
>
>> 300: }
>> 301: return data;
>> 302: }
>
> What is the difference between this method and `HexFormat.of().parseHex(hex)`
> ? We should just reuse existing java APIs if possible.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/9f050b6a1a4a83d8623e206323071c2c77c90bb2.
Please indicate if resolved.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1729544932
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1729544857
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1729544711
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1729544504
