<div dir="ltr"><div>Hi Alan,<br><br></div>thanks a lot for the fast review and your valuable comments. Please find my answers inline:<br><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Nov 21, 2013 at 1:01 PM, Alan Bateman <span dir="ltr"><<a href="mailto:Alan.Bateman@oracle.com" target="_blank">Alan.Bateman@oracle.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div class="im">
On 20/11/2013 18:26, Volker Simonis wrote:
<blockquote type="cite">
<div dir="ltr">
<div>
<div>Hi,<br>
<br>
this is the second review round for "<a 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.
<div><span></span><span></span></div>
</div>
</div>
</div>
</blockquote></div>
Thanks for the update and addressing all the original comments and
suggestions. In particular, moving most of the AIX specific files to
src/aix and including an implementation of dladdr, make a big
difference and makes this much easier to review.<br>
<br>
I've skimmed through all the non-client files in the webrev and just
have a few comments:<br>
<br>
NetworkLibraries.gmk - is the exclude of bsd_close.c right? It looks
like it will add this to LIBNET_EXCLUDE_FILES even when building on
Mac.<br>
<br></div></blockquote><div><br>You're right, that's a typo. That should have read:<br><pre><span class=""> 48 ifneq ($(OPENJDK_TARGET_OS), aix)</span>
<span class=""> 49 LIBNET_EXCLUDE_FILES += aix_close.c</span>
<span class=""><span class=""> 50 else</span>
<span class=""> 51 LIBNET_SRC_DIRS += $(JDK_TOPDIR)/src/aix/native/java/net/</span>
52 endif</span>
</pre></div><div>But actually I've just realized that it is not need at all, because 'aix_close.c' isn't in the PATH for any other OS than AIX (that could be probably called a feature of the new file layout:) So I'll simply change it to:<br>
<pre><span class=""> 48 ifeq ($(OPENJDK_TARGET_OS), aix)</span>
<span class=""><span class=""> 49 LIBNET_SRC_DIRS += $(JDK_TOPDIR)/src/aix/native/java/net/</span>
50 endif</span></pre></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
In the old verifier code (check_code.c) then it's not clear to me
that the caller wrapper is needed but in any case the change
suggests to me that we should look at the malloc usages so that they
better handle the size==0 case. I realize the wrappers are to avoid
changing too much and it should be okay to handle this via a
separate bug.<br>
<br></div></blockquote><div><br>Yes, exactly. I didn't wanted to change too much code. But as the C-Standard states (<a href="http://pubs.opengroup.org/onlinepubs/000095399/functions/malloc.html">http://pubs.opengroup.org/onlinepubs/000095399/functions/malloc.html</a>) "...If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() shall be returned..." it is perfectly legal that malloc/calloc return a NULL pointer if called with a zero argument. This case is currently not handled (i.e. it's handled as an 'out of memory' error) in check_code.c and I agree that this should be fixed via a separate bug.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
In net_util.c then it's a bit ugly to be calling aix_close_init.
Michael/Chris - what you would think about the JNI_OnLoad calling
into a platform specific function to do platform specific
initialization?<br>
<br></div></blockquote><div><br></div><div>What about renaming 'initLocalAddrTable()' into something like 'platformInit()' and moving the call to 'aix_close_init' to a AIX-specific version of 'platformInit()' in net_util_md.c?<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
The changes to java_md_solinux.c look okay to me but it makes me
wonder if this should be renamed as it no longer exclusively Solaris
+ Linux.<br>
<br></div></blockquote><div><br></div><div>You're right - we could rename it to something like 'java_md_unix.c'. But no matter how fancy the name would be, the file would still be in the 'src/solaris/bin' subdirectory:( So I think we'd better leave this for a later change when we completely factor out the Linux/Mac code from the 'solaris/' directory.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
Port.java - one suggestion for unregisterImpl is to rename it to
preUnregister and change it to protected so that it's more obvious
that it supposed to be overridden.<br>
<br></div></blockquote><div><br></div><div>Done. Also changed the comment to JavaDoc style to be more consistent with the other comments in that file.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
UnixNativeDispatcher.c - this looks okay (must reduced since the
first round), I just wonder if the changes to *_getpwuid and
*_getgrgid are really needed as this just impacts the error message.
Also might be good to indent the #ifdef to be consistent with the
other usages in these functions.<br>
<br></div></blockquote><div><br></div><div>You're right. This change was done before you fixed "7043788: (fs) PosixFileAttributes.owner() or group() throws NPE if owner/group not in passwd/group database" (<a href="http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/f91c799f7bfb">http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/f91c799f7bfb</a>). After you're fix it was "automatically" adapted. I've removed the special AIX handling as suggested because I think as well that another error message in the exception won't have any impact.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
That's mostly it. I notice that only a small number of tests have
been updated. Are there more test updates to come? I'm pretty sure
we have a lot more tests that may require update (searching for
SunOS might give some hints).<span class=""><font color="#888888"><br>
<br></font></span></div></blockquote><div><br></div><div>I'm currently working on it and created "8028537: PPC64: Updated jdk/test scripts to understand the AIX os and environment" for it because I didn't wanted to blow up this change unnecessarily. <br>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><span class=""><font color="#888888">
-Alan.<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
</font></span></div>
</blockquote></div><br></div></div></div></div>