8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Ivan Gerasimov]

Hi Brian!

Inet4AddressImpl.c:

1) There is an extra space after FormatMessage,

2) It is preexisting. The code doesn't check if FormatMessage failed to 
allocate the buffer.
It's not clear from the MSDN documentation, if the pointer to the buffer 
will be set to NULL upon the failure.
If it does not, then subsequent NET_ThrowNew(env, err, buf); 
LocalFree(buf); may hit uninitialized memory.
It would be more accurate to invoke them only if (n > 0).

3) It purely optional, but you may want to use the TEXT macro to append 
the L prefix to the character literals, if TCHAR is defined to be WCHAR:

  389                     if (buf[n - 1] == TEXT('\n')) n--;
  390                     if (buf[n - 1] == TEXT('\r')) n--;
  391                     if (buf[n - 1] == TEXT('.')) n--;
  392                     buf[n] = TEXT('\0');

It may make the compiler just a tiny bit happier :)

Everything else looks good to me.

With kind regards,
Ivan

On 6/11/19 10:56 AM, Brian Burkhalter wrote:
> https://bugs.openjdk.java.net/browse/JDK-8223813
> http://cr.openjdk.java.net/~bpb/8223813/webrev.00/ 
> <http://cr.openjdk.java.net/%7Ebpb/8223813/webrev.00/>
>
> FormatMessage() and FormatMessageW() occur in a number of locations:
>
> src/java.base/windows/native/libjli/java_md.c
> src/java.base/windows/native/libnet/Inet4AddressImpl.c
> src/java.base/windows/native/libjava/ProcessImpl_md.c
> src/java.base/windows/native/libjava/jni_util_md.c
> src/java.base/windows/native/libnio/ch/Iocp.c
> src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c
> src/java.base/share/native/libzip/zlib/gzlib.c
>
> Some of these already strip the terminal CRLF (or dot + CRLF) of the 
> string populated by FormatMessage[W](). This patch would add removing 
> them, if present, from
>
> src/java.base/windows/native/libnet/Inet4AddressImpl.c
> src/java.base/windows/native/libnio/ch/Iocp.c
> src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c
>
> One question is whether it would be better just to consolidate this 
> code into two methods for example in jni_uitl and call the methods 
> from the other locations. There are already getLastErrorString() and 
> getErrorString() for chars here.
>
> Also, I am not sure how to test this effectively. The code passes all 
> tiers as-is.
>
> Thanks,
>
> Brian

-- 
With kind regards,
Ivan Gerasimov

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190611/f0799ad7/attachment.html>