RFR: 8369995: Implement extra logging and/or propagate errors in X509KeyManagerImpl and fix StringIndexOutOfBoundsException [v6]

[Bradford Wetmore]

On Mon, 20 Oct 2025 14:28:39 GMT, Artur Barashev <abarashev at openjdk.org> wrote:

>> Mikhail Yankelevich has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Artur's comments
>
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 227:
> 
>> 225:         int secondDot = alias.indexOf('.', firstDot + 1);
>> 226: 
>> 227:         if (firstDot < 1 ||
> 
> Nit: `||` should be first on the new line per style guide.

Nit:  I prefer to (over)use commas to indicate exactly what you intended.

Artur is right on the `||`, but I am guilty of doing it your way.  :)

That said, the next lines in a conditional should be indented by 8.  

        if ((firstDot < 1) || (secondDot - firstDot < 2) ||
                (alias.length() - secondDot < 2))
            if (SSLLogger.isOn && SSLLogger.isOn("keymanager")) {
                SSLLogger.warning("Invalid alias format: " + alias);
            }

> test/jdk/sun/security/ssl/X509KeyManager/NullCases.java line 41:
> 
>> 39:  *
>> 40:  * @run junit NullCases
>> 41:  * @run junit/othervm -Djavax.net.debug=ssl:keymanager -Darg=debug NullCases
> 
> Why do we have a 2nd run with debugging on?

I am in the process of disabling extra (unneeded) debug output.  It looks like there is a good reason to run with debug to check the logging output here, but is there a reason to do an initial run **without** the debugging on?

> test/jdk/sun/security/ssl/X509KeyManager/NullCases.java line 68:
> 
>> 66: 
>> 67: public class NullCases {
>> 68:     private static final String KEY_MGR_EXCEPTION_MESSAGE = "Exception thrown while getting an alias";
> 
> Line length.

I noticed several other cases here.  Please keep line lengths to <=80.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2445485110
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2445494979
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2445498509