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

[Chris Hegarty]

This latest version looks good to me.

-Chris

> On 19 Jun 2015, at 19:07, Roger Riggs <Roger.Riggs at oracle.com> wrote:
> 
> Hi Ivan,
> 
> Both fixed,
> 
> Thanks, Roger
> 
>> On 6/19/2015 1:33 PM, Ivan Gerasimov wrote:
>> 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.
>