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

[Roger Riggs]

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