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.
> >>>
> >>>
> >>>
> >>
