On 15 jan 2014, at 18:52, Volker Simonis <volker.simo...@gmail.com> wrote:
> > > On Wed, Jan 15, 2014 at 6:27 PM, Volker Simonis <volker.simo...@gmail.com> > wrote: > > On Wed, Jan 15, 2014 at 5:34 PM, Volker Simonis > > <volker.simo...@gmail.com> wrote: > >> Hi Staffan, > >> > >> thanks for the review. Please find my comments inline: > >> > >> On Wed, Jan 15, 2014 at 9:57 AM, Staffan Larsen <staffan.lar...@oracle.com> > >> wrote: > >>> > >>> Volker, > >>> > >>> I’ve look at the following files: > >>> > >>> src/share/native/sun/management/DiagnosticCommandImpl.c: > >>> nit: “legel” -> “legal” (two times) > >>> In Java_sun_management_DiagnosticCommandImpl_getDiagnosticCommandInfo() if > >>> you allow dcmd_info_array to become NULL, then > >>> jmm_interface->GetDiagnosticCommandInfo() will throw an NPE and you need > >>> to > >>> check that. > >> > >> > >> Good catch. I actually had problems with malloc returning NULL in > >> 'getDiagnosticCommandArgumentInfoArray()' and then changed all other > >> potentially dangerous locations which used the same pattern. > >> > >> However I think if the 'dcmd_info_array' has zero length it would be > >> perfectly fine to return a zero length array. So what about the following > >> solution: > >> > > Sorry for the noise - it seems I was a little indisposed during the last > mails:) > So this is the simple change I'd like to propose for > Java_sun_management_DiagnosticCommandImpl_getDiagnosticCommandInfo(): > > > @@ -117,19 +119,23 @@ > return NULL; > } > num_commands = (*env)->GetArrayLength(env, commands); > - dcmd_info_array = (dcmdInfo*) malloc(num_commands * > - sizeof(dcmdInfo)); > + dcmdInfoCls = (*env)->FindClass(env, > + "sun/management/DiagnosticCommandInfo"); > + result = (*env)->NewObjectArray(env, num_commands, dcmdInfoCls, NULL); > + if (result == NULL) { > + JNU_ThrowOutOfMemoryError(env, 0); > + } > + if (num_commands == 0) { > + /* Handle the 'zero commands' case specially to avoid calling > 'malloc()' */ > + /* with a zero argument because that may legally return a NULL > pointer. */ > + return result; > + } > + dcmd_info_array = (dcmdInfo*) malloc(num_commands * sizeof(dcmdInfo)); > if (dcmd_info_array == NULL) { > JNU_ThrowOutOfMemoryError(env, NULL); > } > jmm_interface->GetDiagnosticCommandInfo(env, commands, dcmd_info_array); > - dcmdInfoCls = (*env)->FindClass(env, > - "sun/management/DiagnosticCommandInfo"); > - result = (*env)->NewObjectArray(env, num_commands, dcmdInfoCls, NULL); > - if (result == NULL) { > - free(dcmd_info_array); > - JNU_ThrowOutOfMemoryError(env, 0); > - } > for (i=0; i<num_commands; i++) { > args = getDiagnosticCommandArgumentInfoArray(env, > > (*env)->GetObjectArrayElement(env,commands,i), > > If the 'commands' input array is of zero length just return a zero length > array. > OK? Yes, this looks good (still :-) ) /Staffan > > >> dcmdInfoCls = (*env)->FindClass(env, > >> "sun/management/DiagnosticCommandInfo"); > >> num_commands = (*env)->GetArrayLength(env, commands); > > > > Sorry, of course I wanted to say "if (num_commands == 0)" here! > > > >> if (num_commands = 0) { > >> result = (*env)->NewObjectArray(env, 0, dcmdInfoCls, NULL); > >> if (result == NULL) { > >> JNU_ThrowOutOfMemoryError(env, 0); > >> } > >> else { > >> return result; > >> } > >> } > >> dcmd_info_array = (dcmdInfo*) malloc(num_commands * sizeof(dcmdInfo)); > >> if (dcmd_info_array == NULL) { > >> JNU_ThrowOutOfMemoryError(env, NULL); > >> } > >> jmm_interface->GetDiagnosticCommandInfo(env, commands, dcmd_info_array); > >> result = (*env)->NewObjectArray(env, num_commands, dcmdInfoCls, NULL); > >> > >> That seems easier and saves me from handling the exception. > >> > >> What do you think? > >> > >>> src/solaris/native/sun/management/OperatingSystemImpl.c > >>> No comments. > >>> > >>> src/share/transport/socket/socketTransport.c > >>> No comments. > >>> > >>> > >>> src/share/classes/sun/tools/attach/META-INF/services/com.sun.tools.attach.spi.AttachProvider > >>> No comments. > >>> > >>> > >>> Thanks, > >>> /Staffan > >>> > >>> > >>> > >>> On 14 jan 2014, at 09:40, Volker Simonis <volker.simo...@gmail.com> wrote: > >>> > >>> Hi, > >>> > >>> could you please review the following changes for the ppc-aix-port > >>> stage/stage-9 repositories (the changes are planned for integration into > >>> ppc-aix-port/stage-9 and subsequent backporting to ppc-aix-port/stage): > >>> > >>> http://cr.openjdk.java.net/~simonis/webrevs/8031581/ > >>> > >>> I've build and smoke tested without any problems on Linux/x86_64 and > >>> PPC64, Windows/x86_64, MacOSX, Solaris/SPARC64 and AIX7PPC64. > >>> > >>> With these changes (and together with the changes from "8028537: PPC64: > >>> Updated jdk/test scripts to understand the AIX os and environment" and > >>> "8031134 : PPC64: implement printing on AIX") our port passes all but the > >>> following 7 jtreg regression tests on AIX (compared to the Linux/x86_64 > >>> baseline from www.java.net/download/jdk8/testresults/testresults.html): > >>> > >>> java/net/Inet6Address/B6558853.java > >>> java/nio/channels/AsynchronousChannelGroup/Basic.java (sporadically) > >>> java/nio/channels/AsynchronousChannelGroup/GroupOfOne.java > >>> java/nio/channels/AsynchronousChannelGroup/Unbounded.java (sporadically) > >>> java/nio/channels/Selector/RacyDeregister.java > >>> sun/security/krb5/auto/Unreachable.java (only on IPv6) > >>> > >>> Thank you and best regards, > >>> Volker > >>> > >>> > >>> Following a detailed description of the various changes: > >>> > >>> src/share/native/java/util/zip/zip_util.c > >>> src/share/native/sun/management/DiagnosticCommandImpl.c > >>> > >>> According to ISO C it is perfectly legal for malloc to return zero if > >>> called with a zero argument. Fix various places where malloc can > >>> potentially > >>> correctly return zero because it was called with a zero argument. > >>> Also fixed DiagnosticCommandImpl.c to include stdlib.h. This only fixes a > >>> compiler warning on Linux, but on AIX it prevents a VM crash later on > >>> because the return value of malloc() will be casted to int which is > >>> especially bad if that pointer was bigger than 32-bit. > >>> > >>> make/CompileJavaClasses.gmk > >>> > >>> Also use PollingWatchService on AIX. > >>> > >>> make/lib/NioLibraries.gmk > >>> src/aix/native/sun/nio/ch/AixNativeThread.c > >>> > >>> Put the implementation for the native methods of NativeThread into > >>> AixNativeThread.c on AIX. > >>> > >>> src/solaris/native/sun/nio/ch/PollArrayWrapper.c > >>> src/solaris/native/sun/nio/ch/Net.c > >>> src/aix/classes/sun/nio/ch/AixPollPort.java > >>> src/aix/native/sun/nio/ch/AixPollPort.c > >>> src/aix/native/java/net/aix_close.c > >>> > >>> On AIX, the constants used for the polling events (i.e. POLLIN, POLLOUT, > >>> ...) are defined to different values than on other operating systems. The > >>> problem is however, that these constants are hardcoded as public final > >>> static members of various, shared Java classes. We therefore have to map > >>> them from Java to native every time before calling one of the native poll > >>> functions and back to Java after the call on AIX in order to get the right > >>> semantics. > >>> > >>> src/share/classes/java/nio/file/CopyMoveHelper.java > >>> > >>> As discussed on the core-libs mailing list (see > >>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024119.html) > >>> it is not necessary to call Files.getFileAttributeView() with any > >>> linkOptions because at that place we've already checked that the target > >>> file > >>> can not be a symbolic link. This change makes the implementation more > >>> robust > >>> on platforms which support symbolic links but do not support the > >>> O_NOFOLLOW > >>> flag to the open system call. It also makes the JDK pass the > >>> demo/zipfs/basic.sh test on AIX. > >>> > >>> src/share/classes/sun/nio/cs/ext/ExtendedCharsets.java > >>> > >>> Support "compound text" on AIX in the same way like on other Unix > >>> platforms. > >>> > >>> > >>> src/share/classes/sun/tools/attach/META-INF/services/com.sun.tools.attach.spi.AttachProvider > >>> > >>> Define the correct attach provider for AIX. > >>> > >>> src/solaris/native/java/net/net_util_md.h > >>> src/solaris/native/sun/nio/ch/FileDispatcherImpl.c > >>> src/solaris/native/sun/nio/ch/ServerSocketChannelImpl.c > >>> > >>> AIX needs a workaround for I/O cancellation (see: > >>> http://publib.boulder.ibm.com/infocenter/pseries/v5r3/index.jsp?topic=/com.ibm.aix.basetechref/doc/basetrf1/close.htm). > >>> "..The close() subroutine is blocked until all subroutines which use the > >>> file descriptor return to usr space. For example, when a thread is calling > >>> close and another thread is calling select with the same file descriptor, > >>> the close subroutine does not return until the select call returns...". To > >>> fix this problem, we have to use the various NET_ wrappers which are > >>> declared in net_util_md.h and defined in aix_close.c and we also need some > >>> additional wrappers for fcntl(), read() and write() on AIX. > >>> While the current solution isn't really nice because it introduces some > >>> more AIX-specifc sections in shared code, I think it is the best way to go > >>> for JDK 8 because it imposes the smallest possible changes and risks for > >>> the > >>> existing platforms. I'm ready to change the code to unconditionally use > >>> the > >>> wrappers for all platforms and implement the wrappers empty on platforms > >>> which don't need any wrapping. I think it would also be nice to clean up > >>> the > >>> names (e.g. NET_Read() is currently a wrapper for recv() and the NET_ > >>> prefix > >>> is probably not appropriate any more so maybe change it to something like > >>> IO_). But again, I'll prefer to keep that as a follow up change for JDK9. > >>> Calling fsync() on a "read-only" file descriptor on AIX will result in an > >>> error (i.e. "EBADF: The FileDescriptor parameter is not a valid file > >>> descriptor open for writing."). To prevent this error we have to query if > >>> the corresponding file descriptor is writeable. Notice that at that point > >>> we > >>> can not access the writable attribute of the corresponding file channel so > >>> we have to use fcntl(). > >>> > >>> src/solaris/classes/java/lang/UNIXProcess.java.aix > >>> > >>> On AIX the implementation is especially tricky, because the close() system > >>> call will block if another thread is at the same time blocked in a file > >>> operation (e.g. 'read()') on the same file descriptor. We therefore > >>> combine > >>> the AIX ProcessPipeInputStream implemenatation with the > >>> DeferredCloseInputStream approach used on Solaris (see > >>> UNIXProcess.java.solaris). This means that every potentially blocking > >>> operation on the file descriptor increments a counter before it is > >>> executed > >>> and decrements it once it finishes. The 'close()' operation will only be > >>> executed if there are no pending operations. Otherwise it is deferred > >>> after > >>> the last pending operation has finished. > >>> > >>> src/share/transport/socket/socketTransport.c > >>> > >>> On AIX we have to call shutdown() on a socket descriptor before closing > >>> it, otherwise the close() call may be blocked. This is the same problem as > >>> described before. Unfortunately the JDI framework doesn't use the same IO > >>> wrappers like other class library components so we can not easily use the > >>> NET_ abstractions from aix_close.c here. > >>> Without this small change all JDI regression tests will fail on AIX > >>> because of the way how the tests act as a "debugger" which launches > >>> another > >>> VM (the "debugge") which connects itself back to the debugger. In this > >>> scenario the "debugge" can not shut down itself because one thread will > >>> always be blocked in the close() call on one of the communication sockets. > >>> > >>> src/solaris/native/java/net/NetworkInterface.c > >>> > >>> Set the scope identifier for IPv6 addresses on AIX. > >>> > >>> src/solaris/native/java/net/net_util_md.c > >>> > >>> It turns out that we do not always have to replace SO_REUSEADDR on AIX by > >>> SO_REUSEPORT. Instead we can simply use the same approach like BSD and > >>> only > >>> use SO_REUSEPORT additionally, if several datagram sockets try to bind to > >>> the same port. > >>> Also fixed a comment and removed unused local variables. > >>> Fixed the obviously inverted assignment newTime = prevTime; which should > >>> read prevTime = newTime;. Otherwise prevTime will never change and the > >>> timeout will be potential reached too fast. > >>> > >>> src/solaris/native/sun/management/OperatingSystemImpl.c > >>> > >>> AIX does not understand /proc/self so we have to query the real process ID > >>> to access the proc file system. > >>> > >>> src/solaris/native/sun/nio/ch/DatagramChannelImpl.c > >>> > >>> On AIX, connect() may legally return EAFNOSUPPORT if called on a socket > >>> with the address family set to AF_UNSPEC. > >>> > >>> > >>> > >>