RFR 9/8: 8066504: GetVersionEx in java.base/windows/native/libjava/java_props_md.c might not get correct Windows version

[Ivan Gerasimov]

Thanks!  I like it better now :)

A couple of minor nits:
- please use VerQueryValueW() for consistency,
- missing space after if in  '507     if(is_workstation'.

Otherwise looks fine to me.

Sincerely yours,
Ivan

On 19.06.2015 16:38, Roger Riggs wrote:
> Hi Ivan,
>
> Thanks for the review and recommendation.
>
> The webrev is updated in place to use WCHAR.
>
> http://cr.openjdk.java.net/~rriggs/webrev-win-ver-8066504/
>
> On 6/18/2015 2:07 PM, Ivan Gerasimov wrote:
>> Hi Roger!
>>
>> On 17.06.2015 17:27, Roger Riggs wrote:
>>> I updated the webrev to remove the Windows 3.1 case and
>>> will wait 24hrs to see if there are more comments.
>>>
>>
>> I have a little concern.
>> You're using char-agnostic versions of functions 
>> GetSystemDirectory(), GetFileVersionInfoSize(), GetFileVersionInfo() 
>> with a plain char buffer.
>> I think it would be better to either declare kernel32_path TCHAR[] or 
>> explicitly use ANSI versions of the functions.
>>
>> Personally, I'd prefer if kernel32_path were WCHAR and Unicode 
>> versions of the functions were used.
>> I don't know if it's allowed to have non-ASCII chars in the 
>> SysDirectory path, but it seems reasonable to assume it is.
> I'll switch to the WCHAR version to avoid any doubt.
>
> Thanks, Roger
>
>>
>> It would be interesting to setup Windows with non-English name of the 
>> system directory and see what's starting to fail :)
>> I'm going to do that as my spare time.
>>
>> Sincerely yours,
>> Ivan
>>
>>
>>> Thanks for the review, Roger
>>>
>>>
>>> On 6/17/2015 9:58 AM, Alan Bateman wrote:
>>>> On 17/06/2015 14:49, Roger Riggs wrote:
>>>>> Hi Alan,
>>>>>
>>>>> Would there be any concerns about backporting to JDK 8?
>>>> I don't think so but it will require the jdk8u maintainers to approve.
>>>>>
>>>>> The new helper APIs 
>>>>> <https://msdn.microsoft.com/en-us/library/windows/desktop/ms724451%28v=vs.85%29.aspx> 
>>>>> only verify that the OS is at least the version requested.
>>>>> It does not reveal what version the OS is actually running on.
>>>>> Adding a manifest attribute just declares the minimum version 
>>>>> required
>>>>> and is reported by GetVersionEx.
>>>>> In the current case, the code uses GetVersionEx as a fallback in
>>>>> case the version on kernel32.dll is not available.
>>>>> That fallback could be removed since kernel32.dll must exist on 
>>>>> all supported versions.
>>>> Okay, I thought the helper APIs had better support than this. Maybe 
>>>> the Microsoft folks that are contributing to OpenJDK might have 
>>>> suggestions. In general I don't have any objections to the 
>>>> approach, just thought it was a big odd that the OS forces us to 
>>>> look at the version of kernel32.dll.
>>>>
>>>>
>>>>> :
>>>>>
>>>>>>
>>>>>> Also, is there any update needed in 
>>>>>> src/windows/resource/java.manifest?
>>>>> If using the version of kernel32.dll then the manifest version 
>>>>> does not affect the value of os.name.
>>>>> But yes, I added the Windows 10 supportedOS uid.
>>>> I don't think this was in the first webrev that I saw but I see it 
>>>> is there now, looks fine.
>>>>
>>>>
>>>>>>
>>>>>> In passing, GetNativeSystemInfo has been in Windows since Windows 
>>>>>> XP so I don't think we need to use GetModuleHandle to get the 
>>>>>> address now.
>>>>> ok.
>>>>>
>>>>> Similarly, can the case for Windows 3.1 be removed?   (its not in 
>>>>> theJDK 8 supported matrix 
>>>>> <http://www.oracle.com/technetwork/java/javase/certconfig-2095354.html>)
>>>>> caseVER_PLATFORM_WIN32s:
>>>> I think that would be a fine cleanup.
>>>>
>>>> -Alan.
>>>
>>>
>>>
>>
>