Hi Alan, thanks a lot for the fast review and your valuable comments. Please find my answers inline:
On Thu, Nov 21, 2013 at 1:01 PM, Alan Bateman <alan.bate...@oracle.com>wrote: > On 20/11/2013 18:26, Volker Simonis wrote: > > Hi, > > this is the second review round for "8024854: Basic changes and files to > build the class library on > AIX<https://bugs.openjdk.java.net/browse/JDK-8024854>". > The previous reviews can be found at the end of this mail in the references > section. > > 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. > > 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. > > I've skimmed through all the non-client files in the webrev and just have > a few comments: > > 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. > > You're right, that's a typo. That should have read: 48 ifneq ($(OPENJDK_TARGET_OS), aix) 49 LIBNET_EXCLUDE_FILES += aix_close.c 50 else 51 LIBNET_SRC_DIRS += $(JDK_TOPDIR)/src/aix/native/java/net/ 52 endif 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: 48 ifeq ($(OPENJDK_TARGET_OS), aix) 49 LIBNET_SRC_DIRS += $(JDK_TOPDIR)/src/aix/native/java/net/ 50 endif 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. > > Yes, exactly. I didn't wanted to change too much code. But as the C-Standard states ( http://pubs.opengroup.org/onlinepubs/000095399/functions/malloc.html) "...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. > 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? > > 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? > 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. > > 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. > 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. > > Done. Also changed the comment to JavaDoc style to be more consistent with the other comments in that file. > 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. > > 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" ( http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/f91c799f7bfb). 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. > 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). > > 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. -Alan. > > > > > > > >
