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