Hi Sean,<div><br></div><div>thanks a lot for you review. </div><div><br></div><div>Please let me know once you start working on 6997010 so I can update the corresponding AIX file accordingly.</div><div><br></div><div>Regards,</div>
<div>Volker<br><br>On Monday, November 25, 2013, Sean Mullan wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Volker,<br>
<br>
The security changes look fine. I'm not crazy that we now have to maintain one additional java.security file which is the exact same as java.security-linux, but this is really an existing issue with duplicated content across the java.security files which I will try to fix early in JDK 9: <a href="https://bugs.openjdk.java.net/browse/JDK-6997010" target="_blank">https://bugs.openjdk.java.net/<u></u>browse/JDK-6997010</a><br>
<br>
Thanks,<br>
Sean<br>
<br>
On 11/20/2013 01:26 PM, Volker Simonis wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<br>
this is the second review round for "8024854: Basic changes and files to<br>
build the class library on<br>
AIX<<a href="https://bugs.openjdk.java.net/browse/JDK-8024854" target="_blank">https://bugs.openjdk.java.<u></u>net/browse/JDK-8024854</a>>".<br>
The previous reviews can be found at the end of this mail in the references<br>
section.<br>
<br>
I've tried to address all the comments and suggestions from the first round<br>
and to further streamline the patch (it perfectly builds on Linux/x86_64,<br>
Linux/PPC664, AIX, Solaris/SPARC and Windows/x86_64). The biggest change<br>
compared to the first review round is the new "aix/" subdirectory which<br>
I've now created under "jdk/src" and which contains AIX-only code.<br>
<br>
The webrev is against our<br>
<a href="http://hg.openjdk.java.net/ppc-aix-port/stagerepository" target="_blank">http://hg.openjdk.java.net/<u></u>ppc-aix-port/stagerepository</a> which has been<br>
recently synchronised with the jdk8 master:<br>
<br>
<a href="http://cr.openjdk.java.net/~simonis/webrevs/8024854.v2/" target="_blank">http://cr.openjdk.java.net/~<u></u>simonis/webrevs/8024854.v2/</a><br>
<br>
Below (and in the webrev) you can find some comments which explain the<br>
changes to each file. In order to be able to push this big change, I need<br>
the approval of reviewers from the lib, net, svc, 2d, awt and sec groups.<br>
So please don't hesitate to jump at it - there are enough changes for<br>
everybody:)<br>
<br>
Thank you and best regards,<br>
Volker<br>
<br>
*References:*<br>
<br>
The following reviews have been posted so far (thanks Iris for collecting<br>
them :)<br>
<br>
- Alan Bateman (lib): Initial comments (16 Sep [2])<br>
- Chris Hegarty (lib/net): Initial comments (20 Sep [3])<br>
- Michael McMahon (net): Initial comments (20 Sept [4])<br>
- Steffan Larsen (svc): APPROVED (20 Sept [5])<br>
- Phil Race (2d): Initial comments (18 Sept [6]); Additional comments (15<br>
Oct [7])<br>
- Sean Mullan (sec): Initial comments (26 Sept [8])<br>
<br>
[2]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001045.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001045.html</a><br>
[3]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001078.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001078.html</a><br>
[4]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001079.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001079.html</a><br>
[5]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001077.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001077.html</a><br>
[6]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001069.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001069.html</a><br>
[7]: <a href="http://mail.openjdk.java.net/pipermail/2d-dev/2013-October/003826.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/2d-dev/2013-October/<u></u>003826.html</a><br>
[8]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001081.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001081.html</a><br>
<br>
<br>
*Detailed change description:*<br>
<br>
The new "jdk/src/aix" subdirectory contains the following new and<br>
AIX-specific files for now:<br>
<br>
src/aix/classes/sun/awt/<u></u>fontconfigs/aix.fontconfig.<u></u>properties<br>
src/aix/classes/sun/nio/ch/<u></u>AixAsynchronousChannelProvider<u></u>.java<br>
src/aix/classes/sun/nio/ch/<u></u>AixPollPort.java<br>
src/aix/classes/sun/nio/fs/<u></u>AixFileStore.java<br>
src/aix/classes/sun/nio/fs/<u></u>AixFileSystem.java<br>
src/aix/classes/sun/nio/fs/<u></u>AixFileSystemProvider.java<br>
src/aix/classes/sun/nio/fs/<u></u>AixNativeDispatcher.java<br>
src/aix/classes/sun/tools/<u></u>attach/AixAttachProvider.java<br>
src/aix/classes/sun/tools/<u></u>attach/AixVirtualMachine.java<br>
src/aix/native/java/net/aix_<u></u>close.c<br>
src/aix/native/sun/nio/ch/<u></u>AixPollPort.c<br>
src/aix/native/sun/nio/fs/<u></u>AixNativeDispatcher.c<br>
src/aix/native/sun/tools/<u></u>attach/AixVirtualMachine.c<br>
src/aix/porting/porting_aix.c<br>
src/aix/porting/porting_aix.h<br>
<br>
src/aix/porting/porting_aix.c<br>
src/aix/porting/porting_aix.h<br>
<br>
- Added these two files for AIX relevant utility code.<br>
- Currently these files only contain an implementation of dladdr which<br>
is not available on AIX.<br>
- In the first review round the existing dladdr users in the code either<br>
called the version from the HotSpot on AIX (<br>
src/solaris/native/sun/awt/<u></u>awt_LoadLibrary.c) or had a private copy of<br>
the whole implementation (src/solaris/demo/jvmti/hprof/<u></u>hprof_md.c). This<br>
is now not necessary any more.<br>
<br>
The new file layout required some small changes to the makefiles to make<br>
them aware of the new directory locations:<br>
<br>
makefiles/CompileDemos.gmk<br>
<br>
- Add an extra argument to SetupJVMTIDemo which can be used to pass<br>
additional source locations.<br>
- Currently this is only used on AIX for the AIX porting utilities which<br>
are required by hprof.<br>
<br>
makefiles/lib/Awt2dLibraries.<u></u>gmk<br>
makefiles/lib/<u></u>ServiceabilityLibraries.gmk<br>
<br>
- On AIX add the sources of the AIX porting utilities to the build. They<br>
are required by src/solaris/native/sun/awt/<u></u>awt_LoadLibrary.c and<br>
src/solaris/demo/jvmti/hprof/<u></u>hprof_md.c because dladdr is not available<br>
on AIX.<br>
<br>
makefiles/lib/NioLibraries.gmk<br>
<br>
- Make the AIX-build aware of the new NIO source locations under<br>
src/aix/native/sun/nio/.<br>
<br>
makefiles/lib/<u></u>NetworkingLibraries.gmk<br>
<br>
- Make the AIX-build aware of the new aix_close.c source location under<br>
src/aix/native/java/net/.<br>
<br>
src/share/bin/jli_util.h<br>
<br>
- Define JLI_Lseek on AIX.<br>
<br>
src/share/lib/security/java.<u></u>security-aix<br>
<br>
- Provide default java.security-aix for AIX based on the latest Linux<br>
version as suggested by Sean Mullan.<br>
<br>
src/share/native/common/check_<u></u>code.c<br>
<br>
- On AIX malloc(0) and calloc(0, ...) return a NULL pointer, which is<br>
legal, but the code in check_code.c does not handles this properly. So<br>
we wrap the two methods on AIX and return a non-NULL pointer even if we<br>
allocate 0 bytes.<br>
<br>
src/share/native/sun/awt/<u></u>medialib/mlib_sys.c<br>
<br>
- malloc always returns 8-byte aligned pointers on AIX as well.<br>
<br>
src/share/native/sun/awt/<u></u>medialib/mlib_types.h<br>
<br>
- Add AIX to the list of known platforms.<br>
<br>
src/share/native/sun/font/<u></u>layout/KernTable.cpp<br>
<br>
- Rename the macro DEBUG to DEBUG_KERN_TABLE because DEBUG is too common<br>
and may be defined in other headers as well as on the command line and<br>
xlc bails out on macro redefinitions with a different value.<br>
<br>
src/share/native/sun/security/<u></u>ec/impl/ecc_impl.h<br>
<br>
- Define required types and macros on AIX.<br>
<br>
src/solaris/back/exec_md.c<br>
<br>
- AIX behaves like Linux in this case so check for it in the Linux<br>
branch.<br>
<br>
src/solaris/bin/java_md_<u></u>solinux.c,<br>
src/solaris/bin/java_md_<u></u>solinux.h<br>
<br>
- On AIX LD_LIBRARY_PATH is called LIBPATH<br>
- Always use LD_LIBRARY_PATH macro instead of using the string "<br>
LD_LIBRARY_PATH" directly to cope with different library path names.<br>
- Add jre/lib/<arch>/jli to the standard library search path on AIX<br>
because the AIX linker doesn't support the -rpath option.<br>
- Replace #ifdef __linux__ by #ifndef __solaris__ because in this case,<br>
AIX behaves like Linux.<br>
- Removed the definition of JVM_DLL, JAVA_DLL and LD_LIBRARY_PATH from<br>
java_md_solinux.h because the macros are redefined in the corresponding<br>
.c files anyway.<br>
<br>
src/aix/classes/sun/awt/<u></u>fontconfigs/aix.fontconfig.<u></u>properties<br>
<br>
- Provide basic fontconfig.propertiesfor AIX.<br>
<br>
src/solaris/classes/java/lang/<u></u>UNIXProcess.java.aix,<br>
src/aix/classes/sun/tools/<u></u>attach/AixAttachProvider.java,<br>
src/aix/classes/sun/tools/<u></u>attach/AixVirtualMachine.java,<br>
src/aix/native/sun/tools/<u></u>attach/AixVirtualMachine.c<br>
<br>
- Provide AIX specific Java versions, mostly based on the corresponding<br>
Linux versions.<br>
<br>
src/solaris/classes/sun/nio/<u></u>ch/<u></u>DefaultAsynchronousChannelProv<u></u>ider.java<br>
src/solaris/classes/sun/nio/<u></u>fs/DefaultFileSystemProvider.<u></u>java<br>
<br>
- Detect AIX operating system and return the corresponding channel and<br>
file system providers.<br>
<br>
src/solaris/classes/sun/nio/<u></u>ch/Port.java<br>
<br>
- Add a callback function unregisterImpl(int fd) for implementations<br>
that need special handling when fd is removed and call it from<br>
unregister(int<br>
fd). By default the implementation of unregisterImpl(int fd) is empty<br>
except for the derived AixPollPort class on AIX.<br>
<br>
src/solaris/demo/jvmti/hprof/<u></u>hprof_md.c<br>
<br>
- Add AIX support. AIX mostly behaves like Linux, with the one exception<br>
that it has no dladdr functionality.<br>
- Use the dladdr implementation from porting_aix.{c,h} on AIX.<br>
<br>
src/solaris/native/com/sun/<u></u>management/<u></u>UnixOperatingSystem_md.c<br>
<br>
- Adapt for AIX (i.e. use libperfstat on AIX to query OS memory).<br>
<br>
src/solaris/native/common/jdk_<u></u>util_md.h<br>
<br>
- Add AIX definitions of the ISNANF and ISNAND macros.<br>
<br>
src/solaris/native/java/io/io_<u></u>util_md.c<br>
<br>
- AIX behaves like Linux in this case so check for it in the Linux<br>
branch.<br>
<br>
src/solaris/native/java/lang/<u></u>UNIXProcess_md.c<br>
<br>
- AIX behaves mostly like Solraris in this case so adjust the ifdefs<br>
accordingly.<br>
<br>
src/solaris/native/java/lang/<u></u>childproc.c<br>
<br>
- AIX does not understand '/proc/self' - it requires the real process ID<br>
to query the proc file system for the current process.<br>
<br>
src/solaris/native/java/net/<u></u>NetworkInterface.c<br>
<br>
- Add AIX support into the Linux branch because AIX mostly behaves like<br>
AIX for IPv4.<br>
- AIX needs a special function to enumerate IPv6 interfaces and to query<br>
the MAC address.<br>
- Moved the declaration of siocgifconfRequest to the beginning a the<br>
function (as recommend by Michael McMahon) to remain C89 compatible.<br>
<br>
src/solaris/native/java/net/<u></u>PlainSocketImpl.c<br>
<br>
- On AIX (like on Solaris) setsockopt will set errno to EINVAL if the<br>
socket is closed. The default error message is then confusing.<br>
<br>
src/aix/native/java/net/aix_<u></u>close.c,<br>
src/share/native/java/net/net_<u></u>util.c<br>
<br>
- As recommended by Michael McMahon and Alan Bateman I've move an<br>
adapted version of linux_close.c to<br>
src/aix/native/java/net/aix_<u></u>close.cbecause we also need the file and<br>
socket wrappers on AIX.<br>
- Compared to the Linux version, we've added the initialization of some<br>
previously uninitialized data structures.<br>
- Also, on AIX we don't have __attribute((constructor)) so we need to<br>
initialize manually (from JNI_OnLoad() in<br>
src/share/native/java/net/net_<u></u>util.c.<br>
<br>
src/solaris/native/java/net/<u></u>net_util_md.h<br>
<br>
- AIX needs the same workaround for I/O cancellation like Linux and<br>
MacOSX.<br>
<br>
src/solaris/native/java/net/<u></u>net_util_md.c<br>
<br>
- SO_REUSEADDR is called SO_REUSEPORT on AIX.<br>
- On AIX we have to ignore failures due to ENOBUFS when setting the<br>
SO_SNDBUF/SO_RCVBUF socket options.<br>
<br>
src/solaris/native/java/util/<u></u>TimeZone_md.c<br>
<br>
- Currently on AIX the only way to get the platform time zone is to read<br>
it from the TZ environment variable.<br>
<br>
src/solaris/native/sun/awt/<u></u>awt_LoadLibrary.c<br>
<br>
- Use the dladdr from porting_aix.{c,h} on AIX.<br>
<br>
src/solaris/native/sun/awt/<u></u>fontpath.c<br>
<br>
- Changed some macros from if !defined(__linux__) to if<br>
defined(__solaris__) because that's their real meaning.<br>
- Add AIX specific fontpath settings and library search paths for<br>
libfontconfig.so.<br>
<br>
src/solaris/native/sun/java2d/<u></u>x11/X11SurfaceData.c<br>
<br>
- Rename the MIN and MAX macros to XSD_MIN and XSD_MAX to avoid name<br>
clashes (MIN and MAX are much too common and thexy are already defined<br>
in some AIX system headers).<br>
<br>
src/solaris/native/sun/java2d/<u></u>x11/XRBackendNative.c<br>
<br>
- Handle AIX like Solaris.<br>
<br>
src/solaris/native/sun/nio/ch/<u></u>Net.c<br>
<br>
- Add AIX-specific includes and constant definitions.<br>
- On AIX "socket extensions for multicast source filters" support<br>
depends on the OS version. Check for this and throw appropriate exceptions<br>
if it is requested but not supported. This is needed to pass<br>
JCK-api/java_nio/channels/<u></u>DatagramChannel/<u></u>DatagramChannel.html#Multicast<br>
<br>
src/solaris/native/sun/nio/fs/<u></u>UnixNativeDispatcher.c<br>
<br>
- On AIX strerror() is not thread-safe so we have to use strerror_r()instead.<br>
- On AIX readdir_r() returns EBADF (i.e. '9') and sets 'result' to NULL<br>
for the directory stream end. Otherwise, 'errno' will contain the error<br>
code.<br>
- Handle some AIX corner cases for the results of the statvfs64() call.<br>
- On AIX getgrgid_r() returns ESRCH if group does not exist.<br>
<br>
src/solaris/native/sun/<u></u>security/pkcs11/j2secmod_md.c<br>
<br>
- Use RTLD_LAZY instead of RTLD_NOLOAD on AIX.<br>
<br>
test/java/lang/ProcessBuilder/<u></u>Basic.java<br>
test/java/lang/ProcessBuilder/<u></u>DestroyTest.java<br>
<br>
- Port this test to AIX and make it more robust (i.e. recognize the "C"<br>
locale as isEnglish(), ignore VM-warnings in the output, make sure the<br>
"grandchild" processes created by the test don't run too long (60s vs.<br>
6666s) because in the case of test problems these processes will pollute<br>
the process space, make sure the test fails with an error and doesn't hang<br>
indefinitley in the case of a problem).<br>
<br>
*Q (Michael McMahon):* Seems to be two macros _AIX and AIX. Is this<br>
intended?<br>
<br>
Well, _AIX is defined in some system headers while AIX is defined by the<br>
build system. This is already used inconsistently (i.e. __linux__ vs. LINUX)<br>
and in general I try to be consistent with the style of the file where I<br>
the changes are. That said, I changes most of the occurences of AIX to _AIX.<br>
<br>
<br>
*Q (Alan Bateman):* There are a few changes for OS/400 in the patch, are<br>
they supposed to be there?<br>
<br>
We currently don't support OS/400 (later renamed into i5/OS and currently<br>
called IBM i) in our OpenJDK port but we do support it in our internel, SAP<br>
JVM build. We stripped out all the extra OS/400 functionality from the port<br>
but in some places where there is common functionality needd by both, AIX<br>
and OS/400 the OS/400 support may still be visible in the OpenJDK port. I<br>
don't think this is a real problem and it helps us to keep our internel<br>
sources in sync with the OpenJDK port. That said, I think I've removed all<br>
the references to OS/400 now.<br>
<br>
</blockquote>
<br>
</blockquote></div>