Review request: JDK-8016579 (process) IOException thrown by ProcessBuilder.start() method is incorrectly encoded (v.2)
Dan Xu
dan.xu at oracle.com
Tue Jul 16 19:07:57 UTC 2013
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!
-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