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

[Ivan Gerasimov]

Thanks Brian!

The webrev looks fine to me.

I think that *most likely* the check if (buf != NULL) will work as 
expected:  buf will only be set to non-NULL value upon success.

However, the documentation for the function FormatMessage states:
"""
If the function fails, the return value is zero.
"""

So, my preference would be if (n > 0).

With kind regards,
Ivan



On 6/11/19 5:26 PM, Brian Burkhalter wrote:
> Hi Ivan,
>
> I updated the patch: 
> http://cr.openjdk.java.net/~bpb/8223813/webrev.01/ 
> <http://cr.openjdk.java.net/%7Ebpb/8223813/webrev.01/>
>
> Please see comments inline below.
>
>> On Jun 11, 2019, at 5:06 PM, Ivan Gerasimov 
>> <ivan.gerasimov at oracle.com <mailto:ivan.gerasimov at oracle.com>> wrote:
>>
>> Inet4AddressImpl.c:
>>
>> 1) There is an extra space after FormatMessage,
>
> Fixed.
>
>> 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).
>
> I put “if (buf != NULL)” instead of “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 :)
>
> So changed.
>
>> Everything else looks good to me.
>
> Thanks for the comments!
>
> Brian

-- 
With kind regards,
Ivan Gerasimov

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