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