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