RFR: 10: 8177522: -XX:OnOutOfMemoryError does not work if supplied twice on windows

[Vladimir Kempik]

Hello

Thanks for looking into this.

Updated webrev: http://cr.openjdk.java.net/~vkempik/8177522/webrev.01

see answers below

27.04.17 10:39, David Holmes пишет:
> Hi Vladimir,
>
> Thanks for fixing this.
>
> On 26/04/2017 7:58 PM, Vladimir Kempik wrote:
>> Hello
>>
>> Please review this fix for bug JDK-8177522
>> <https://bugs.openjdk.java.net/browse/JDK-8177522>
>>
>> The issue is windows only.
>>
>> When two OnOutOfMemoryError options are passed to java, they are
>> combined using '\n' as separator.
>>
>> But when OnError parses the string, it only knows ';' as separator of
>> commands.
>>
>> On nix systems it works fine because the command with '\n' in it are
>> passed to shell where it executed fine.
>>
>> With this patch I'm making windows routine to behave similary to nix,
>> passing command to cmd.exe for execution, I just have to replace all
>> '\n' with '&'.
>
> Overall this approach looks good to me.
>
> os_windows.cpp:
>
> +   char * cmd_prefix = "cmd /C ";
>
> Should that be cmd.exe as in vmError.cpp?
I have made both vmerror and here as "cmd /C" now , cmd and cmd.exe is 
the same thing for windows anyway.
>
> Two nits:
>
> + // now replace all '\n' with '&'
>
> Comment should be indented 2 spaces.
>
> +   while ((substring = strchr (substring, '\n')) != NULL) {
>
> Remove space after strchr.
>
done
> ---
>
> vmError.cpp:
>
> I'm wondering though, do we need to modify the vmError code given we 
> will add the "cmd.exe /C" prefix in fork_and_exec anyway?
I thought it should be similar to other platforms, indicating we are 
launching the command thru the shell/cmd, having consistency.
>
> ---
>
> test/runtime/ErrorHandling/TestOnOutOfMemoryError.java
>
> The test should test both a single -XX:OnOutOfMemoryError, and 
> multiple ones.
>
Done
> Thanks,
> David
> -----
>
Thanks, Vladimir.
>> Such workaround is needed to not break compatibility with existing 
>> systems.
>>
>> Testing: jprt, included testcase.
>>
>> Webrev - http://cr.openjdk.java.net/~vkempik/8177522/webrev.00/
>>
>> Bug - https://bugs.openjdk.java.net/browse/JDK-8177522
>>
>> Thanks, Vladimir
>>