Review request: JDK-8016579 (process) IOException thrown by ProcessBuilder.start() method is incorrectly encoded (v.3)
Dan Xu
dan.xu at oracle.com
Wed Jul 17 18:29:24 UTC 2013
I don't know the difference between (*env)->NewStringUTF(env, buf) and
JNU_NewStringPlatform(env, buf). Does the JNU_NewStringPlatform()
function currently fail to process non-western locale? According to the
code, JNU_NewStringPlatform() is also trying to use the right encoding
for the give string.
Other changes look good to me. Thanks for fixing getLastErrorString
function in io_util_md.c file.
-Dan
On 07/17/2013 08:28 AM, Alexey Utkin wrote:
> Thanks,
>
> Here is new version with Dan's correction:
> http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8016579/webrev.02/
>
> Regards,
> -uta
>
>
> On 7/16/2013 11:07 PM, Dan Xu wrote:
>> I agree. Is jdk_util.c or jni_util.c or any file under native/common/
>> a good place?
>>
>> The fix looks good to me.
>>
>> One small comment is to get the return value of swprintf() in
>> win32Error(). If the return value is -1, an error situation needs to
>> be handled even though it seems very rare. If it is not -1, then get
>> the actual length of utf16_javaMessage. And pass the length of
>> utf16_javaMessage to WideCharToMultiByte() instead of MESSAGE_LENGTH
>> to make the code more efficient. Thanks!
> Accepted.
>>
>> -Dan
>>
>>
>>
>>
>> On 07/16/2013 10:49 AM, Martin Buchholz wrote:
>>> Looks OK to me. As ever, we continue to not have good places within
>>> the JDK itself for C-level infrastructure - win32Error is not
>>> specific to the process api, and should really be pulled into some
>>> common directory - but I don't know of any such.
>>>
>>>
>>> On Tue, Jul 16, 2013 at 9:04 AM, Alexey Utkin
>>> <alexey.utkin at oracle.com <mailto:alexey.utkin at oracle.com>> wrote:
>>>
>>> Here is new version of fix:
>>> http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8016579/webrev.01/
>>> <http://cr.openjdk.java.net/%7Euta/openjdk-webrevs/JDK-8016579/webrev.01/>
>>>
>>> On 7/15/2013 9:08 PM, Martin Buchholz wrote:
>>>> Superficial review:
>>>>
>>>> Looks good mostly.
>>>>
>>>> Historically, switching windows code to use "W" APIs has been a
>>>> big TODO, but was waiting for Win98 de-support.
>>> In java.lang we have passed the half-way: the error reporting
>>> sub-system is in the ASCII world.
>>>>
>>>> Please spell correctly:
>>>> MESAGE_LENGTH
>>> Thanks, I missed it. Fixed.
>>>>
>>>> If errno and GetLastError are two separate error notification
>>>> systems, how do you know which one corresponded to the last
>>>> failure? E.g. if the last failure only set errno, won't the
>>>> error message be via GetLastError(), which is likely to be stale?
>>> As Dan mentioned, the os_lasterror was a copy of JVM
>>> os::lasterror call.
>>> The error message procedure is used for
>>> CreatePipe
>>> CreateProcessW
>>> GetExitCodeProcess
>>> WaitForMultipleObjects
>>> fail report. You are right, all the calls return the problem by
>>> GetLastMessage call.
>>> The function is changed (reduced).
>>>
>>> Here is new version of fix:
>>> http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8016579/webrev.01/
>>> <http://cr.openjdk.java.net/%7Euta/openjdk-webrevs/JDK-8016579/webrev.01/>
>>>
>>> Regards,
>>> -uta
>>>
>>>>
>>>>
>>>>
>>>> On Mon, Jul 15, 2013 at 2:41 AM, Alexey Utkin
>>>> <alexey.utkin at oracle.com <mailto:alexey.utkin at oracle.com>> wrote:
>>>>
>>>> Bug description:
>>>> https://jbs.oracle.com/bugs/browse/JDK-8016579
>>>> http://bugs.sun.com/view_bug.do?bug_id=8016579
>>>>
>>>> Here is the suggested fix:
>>>> http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8016579/webrev.00/
>>>> <http://cr.openjdk.java.net/%7Euta/openjdk-webrevs/JDK-8016579/webrev.00/>
>>>>
>>>> Summary:
>>>> We have THREE locales in action:
>>>> 1. Thread default locale - dictates UNICODE-to-8bit conversion
>>>> 2. OS locale that defines the message localization
>>>> 3. The file name locale
>>>>
>>>> Each locale could be an extended locale, that means that
>>>> text cannot be mapped to 8bit sequence without multibyte
>>>> encoding. VM is ready for that, if text is UTF-8.
>>>> The suggested fix does the work right from the beginning.
>>>>
>>>> Unicode version of JVM call:
>>>> hotspot/src/os/windows/vm/os_windows.cpp:
>>>> size_t os::lasterror(char* buf, size_t len)
>>>> was used as prototype for Unicode error message getter. It
>>>> has to be fixed accordingly as well as
>>>> jdk/src/windows/native/java/io/io_util_md.c
>>>> size_t getLastErrorString(char *buf, size_t len)
>>>>
>>>> The bug contains the attachment
>>>> https://jbs.oracle.com/bugs/secure/attachment/14581/JDK-8016579.txt
>>>> that summarize the fix result in comparison with original
>>>> implementation.
>>>>
>>>> Regards,
>>>> -uta
>>>>
>>>>
>>>
>>>
>>
>
More information about the core-libs-dev
mailing list