On 10/20/2011 09:16 PM, Neil Richards wrote:
On Thu, 2011-10-20 at 17:12 +0800, Jonathan Lu wrote:
On 10/20/2011 02:35 PM, Steve Poole wrote:
Thanks Steve, I've updated the test case and patch, see the attachments.
I've added IBM portions copyright comment to both headers.
- Jonathan
For ease of review, I've uploaded the suggested fix as a webrev [1].
Whilst creating this, I stripped the (large amount of) extraneous
trailing whitespace characters from the lines added by the patch.
(It would be helpful to check for this when posting patches).
Thanks Neil for creating that webrev.
I'm sorry for the whitespace issue, I did not 'set list' in my VIM
editor before, will turn on that option from now on. Attachment
patch.diff is an updated version.
Looking at the change, I have some concerns.
Firstly, the added AIX-specific code (between lines 1102-1187) has been
placed in a block of code that looks to be Linux-specific (lines
1013-1365, protected by '#ifdef __linux__').
So it's unclear why any of this code will be used by AIX.
To be clear, I've moved the AIX code of enumIPv6Interfaces to a stand
alone '#ifdef AIX' block.
Secondly, if an exception occurs whilst the list of interfaces is being
compiled, both Linux and Solaris look to return the list compiled up to
the point of the exception (blocks starting at lines 1222& 1505).
In the suggested AIX code, however, the compiled interface list is freed
up in this situation, and NULL returned (block starting at line 1174).
It seems to me that these routines should present similar semantics
(across the 3 platforms).
As the interface list returned is an augmentation of an interface list
that is originally given to the function (the original value of 'ifs'),
the suggested AIX code may free off entries in the list that it was not
responsible for allocating, which runs the risk of resulting in
duplicate calls to free().
Could you please investigate where the responsibilities properly lie for
allocation and deallocation of the entries in the interface list given
to the enumIPv6Interfaces() function, then update the proposal /
comments appropriately?
For the exception handling part, I think AIX implementation works in the
same way as enumIPv4Interfaces on Linux platform.
Although the interface list returned is an argument given to this
function (known as 'ifs'), this pointer is modified by addif() function
which may add a new node to the head of the list, so the value of this
pointer changes in this function.
And if NULL is return by AIX version enumIPv6Interfaces(), then the
interface list pointer 'ifs' from caller function (here is
'enumInterfaces') will be set to NULL which will just skip the next
freeif() operation. So from my point of view, I do not see any potential
duplicate calls to free().
Also, I'm concerned that the name of the testcase provided is overly
vague (what _specifically_ about NetworkInterface does it test?), and
that the @summary description does not describe what the intention of
the test is.
In particular, there is nothing platform-specific about the testcase, so
I would not expect its description to be in terms of something
AIX-specific.
Yes, the testcase is nothing platform-specific, I've updated the name
and summary.
How about changing the name to NetworkInterfaceUniquenessTest?
Hope this helps,
Regards, Neil
[1]http://cr.openjdk.java.net/~ngmr/ojdk-172/webrev.00/
/*
* Copyright (c) 2011 Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
/*
* Portions Copyright (c) 2011 IBM Corporation
*/
import java.net.*;
import java.util.*;
/* @test NetworkInterfaceTest
* @summary This test checks if the interface's addresses returned by getNetworkInterfaces are unique
*/
public class NetworkInterfaceUniquenessTest {
java.net.NetworkInterface pnics;
java.util.Enumeration nics1;
NetworkInterfaceTest() throws Exception{
java.net.NetworkInterface jnic;
java.util.Enumeration<NetworkInterface> nics;
List lst = new ArrayList();
List lst1 = new ArrayList();
try {
nics = java.net.NetworkInterface.getNetworkInterfaces();
while (nics.hasMoreElements()) {
jnic = (java.net.NetworkInterface) nics.nextElement();
lst = jnic.getInterfaceAddresses();
nics1 = jnic.getInetAddresses();
while (nics1.hasMoreElements()) {
InetAddress addr1 = (InetAddress) nics1.nextElement();
if (!(lst1.contains(addr1))) {
lst1.add(addr1);
}
}
if(lst.size() != lst1.size()){
throw new Exception("Duplicate addresses found!");
}
lst1.clear();
}
} catch (java.lang.Exception e) {
e.printStackTrace();
}
}
public static void main(String[] args) throws Exception{
new NetworkInterfaceTest();
}
}
diff -r 4788745572ef src/solaris/native/java/net/NetworkInterface.c
--- a/src/solaris/native/java/net/NetworkInterface.c Mon Oct 17 19:06:53 2011 -0700
+++ b/src/solaris/native/java/net/NetworkInterface.c Fri Oct 21 17:22:43 2011 +0800
@@ -23,6 +23,13 @@
* questions.
*/
+/*
+ * Portions Copyright (c) 2011 IBM Corporation
+ */
+
+#define MAX(x,y) ((x) > (y) ? (x) : (y))
+#define SIZE(p) MAX((p).sa_len, sizeof(p))
+
#include <errno.h>
#include <strings.h>
@@ -1276,6 +1283,93 @@
#endif
+#if defined(AIX) && defined(AF_INET6)
+/*
+ * Enumerates and returns all IPv6 interfaces on AIX
+ */
+static netif *enumIPv6Interfaces(JNIEnv *env, int sock, netif *ifs) {
+ struct ifconf ifc;
+ struct ifreq *ifreqP;
+ char *buf;
+ int numifs;
+ unsigned i;
+ unsigned bufsize;
+ char *cp, *cplimit;
+
+ /* use SIOCGSIZIFCONF to get size for SIOCGIFCONF */
+
+ ifc.ifc_buf = NULL;
+ if (ioctl(sock, SIOCGSIZIFCONF, &(ifc.ifc_len)) < 0) {
+ NET_ThrowByNameWithLastError(env , JNU_JAVANETPKG "SocketException",
+ "ioctl SIOCGSIZIFCONF failed");
+ return ifs;
+ }
+ bufsize = ifc.ifc_len;
+
+ buf = (char *)malloc(bufsize);
+ if (!buf) {
+ JNU_ThrowOutOfMemoryError(env, "Network interface native buffer allocation failed");
+ return ifs;
+ }
+ ifc.ifc_len = bufsize;
+ ifc.ifc_buf = buf;
+ if (ioctl(sock, SIOCGIFCONF, (char *)&ifc) < 0) {
+ NET_ThrowByNameWithLastError(env , JNU_JAVANETPKG "SocketException",
+ "ioctl CSIOCGIFCONF failed");
+ (void) free(buf);
+ return ifs;
+ }
+
+ /*
+ * Iterate through each interface
+ */
+ ifreqP = ifc.ifc_req;
+ cp=(char *)ifc.ifc_req;
+ cplimit=cp+ifc.ifc_len;
+ for (;cp<cplimit;cp+=(sizeof(ifreqP->ifr_name) + SIZE(ifreqP->ifr_addr))){
+ ifreqP=(struct ifreq *)cp;
+ struct ifreq if2;
+
+ memset((char *)&if2, 0, sizeof(if2));
+ strcpy(if2.ifr_name, ifreqP->ifr_name);
+
+ /*
+ * Skip interface that aren't UP
+ */
+ if (ioctl(sock, SIOCGIFFLAGS, (char *)&if2) >= 0) {
+ if (!(if2.ifr_flags & IFF_UP)) {
+ continue;
+ }
+ }
+
+ if (ifreqP->ifr_addr.sa_family != AF_INET6)
+ continue;
+
+ /*
+ * Add to the list
+ */
+ ifs = addif(env, sock, ifreqP->ifr_name, ifs,
+ (struct sockaddr *)&(ifreqP->ifr_addr),
+ AF_INET6,0,-1);
+
+ /*
+ * If an exception occurred then free the list
+ */
+ if ((*env)->ExceptionOccurred(env)) {
+ free(buf);
+ freeif(ifs);
+ return NULL;
+ }
+ }
+
+ /*
+ * Free socket and buffer
+ */
+ free(buf);
+ return ifs;
+}
+#endif
+
/** Solaris **/
#ifdef __solaris__
/* Open socket for further ioct calls, try v4 socket first and
