RFR: 8302659: Modernize Windows native code for NetworkInterface [v2]

[Daniel Jeliński]

On Thu, 16 Feb 2023 16:49:09 GMT, Rich DiCroce <duke at openjdk.org> wrote:

>> Improves performance and correctness, as discussed on the net-dev mailing list.
>
> Rich DiCroce has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Limit line length to 80-ish characters

Overall the changes look good. A few comments below.

src/java.base/windows/native/libnet/NetworkInterface.c line 2:

> 1: /*
> 2:  * Copyright (c) 2003, 2022, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2000, 2023, Oracle and/or its affiliates. All rights reserved.

src/java.base/windows/native/libnet/NetworkInterface.c line 62:

> 60: typedef struct _netaddr {
> 61:     SOCKADDR_INET Address;
> 62:     SCOPE_ID ScopeId;

Not used

src/java.base/windows/native/libnet/NetworkInterface.c line 67:

> 65: } netaddr;
> 66: 
> 67: BOOL getAddressTables(MIB_UNICASTIPADDRESS_TABLE **uniAddrs, MIB_ANYCASTIPADDRESS_TABLE **anyAddrs) {

Please mark all functions that are local to this file as `static`, this will help the compiler

src/java.base/windows/native/libnet/NetworkInterface.c line 69:

> 67: BOOL getAddressTables(MIB_UNICASTIPADDRESS_TABLE **uniAddrs, MIB_ANYCASTIPADDRESS_TABLE **anyAddrs) {
> 68:     ADDRESS_FAMILY addrFamily = ipv6_available() ? AF_UNSPEC : AF_INET;
> 69:     if (GetUnicastIpAddressTable(addrFamily, uniAddrs) != NO_ERROR) {

Please add error handling to all WinAPI calls; something like:

NET_ThrowByNameWithLastError(env, JNU_JAVANETPKG "SocketException", "GetUnicastIpAddressTable");

You may also need to use `SetLastError(ret)` before calling NET_Throw...; the WinAPI functions are not documented to do it, and we do not have a function that would take the error code as a parameter.

src/java.base/windows/native/libnet/NetworkInterface.c line 72:

> 70:         return FALSE;
> 71:     }
> 72:     if (GetAnycastIpAddressTable(addrFamily, anyAddrs) != NO_ERROR) {

Were the anycast addresses covered by the existing code, or is this a functional change?

src/java.base/windows/native/libnet/NetworkInterface.c line 393:

> 391:  * Signature: ()[Ljava/net/NetworkInterface;
> 392:  */
> 393: JNIEXPORT jobjectArray JNICALL Java_java_net_NetworkInterface_getAll(JNIEnv *env, jclass cls) {

This may have also fixed [JDK-6798979](https://bugs.openjdk.org/browse/JDK-6798979) and [JDK-8165665](https://bugs.openjdk.org/browse/JDK-8165665) (which is a good thing!)

src/java.base/windows/native/libnet/NetworkInterface.c line 523:

> 521:  * Signature: (Ljava/lang/String;I)Z
> 522:  */
> 523: JNIEXPORT jboolean JNICALL Java_java_net_NetworkInterface_supportsMulticast0(JNIEnv *env, jclass cls, jstring name, jint index) {

I'm not a big fan of using WBEM here. Also, I'm still trying to figure out when (if ever) this method is supposed to return false

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

PR: https://git.openjdk.org/jdk/pull/12593