Review request: JDK-8016579 (process) IOException thrown by ProcessBuilder.start() method is incorrectly encoded (v.3)

[Alexey Utkin]

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