RFR: 8350869: os::stat doesn't follow symlinks on Windows [v2]

[Anton Artemov]

On Thu, 15 May 2025 07:50:28 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8350869: Addressed reviewer's comments.
>
> src/hotspot/os/windows/os_windows.cpp line 4615:
> 
>> 4613: // This method checks if a wide path is actually a symbolic link
>> 4614: static bool is_symbolic_link(const wchar_t* wide_path)
>> 4615: {
> 
> Suggestion:
> 
> static bool is_symbolic_link(const wchar_t* wide_path) {
> 
> Hotspot-style says the { goes on the same line as the function declaration.

Addressed in the latest commit.

> src/hotspot/os/windows/os_windows.cpp line 4619:
> 
>> 4617:   HANDLE f = ::FindFirstFileW(wide_path, &fd);
>> 4618:   const bool result = fd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT && fd.dwReserved0 == IO_REPARSE_TAG_SYMLINK;
>> 4619:   if (0 == ::FindClose(f)) {
> 
> ```style-nit suggestion
>   if (::FindClose(f) == 0) {

Addressed in the latest commit.

> src/hotspot/os/windows/os_windows.cpp line 4632:
> 
>> 4630: 
>> 4631:   hFile = CreateFileW(wide_path, GENERIC_READ, FILE_SHARE_READ, nullptr,
>> 4632:     OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
> 
> Suggestion:
> 
>   HANDLE hFile = CreateFileW(wide_path, GENERIC_READ, FILE_SHARE_READ, nullptr,
>                              OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);

Addressed in the latest commit.

> test/hotspot/jtreg/runtime/LoadClass/TestSymlinkLoad.java line 68:
> 
>> 66:         OutputAnalyzer output = new OutputAnalyzer(pb.start());
>> 67:         output.shouldContain("Hello World")
>> 68:                 .shouldHaveExitValue(0);
> 
> Suggestion:
> 
>         output.shouldContain("Hello World")
>               .shouldHaveExitValue(0);

Addressed in the latest commit.

> test/hotspot/jtreg/runtime/LoadClass/TestSymlinkLoad.java line 81:
> 
>> 79:         output = new OutputAnalyzer(pb.start());
>> 80:         output.shouldContain("Hello World")
>> 81:                 .shouldHaveExitValue(0);
> 
> Suggestion:
> 
>         output.shouldContain("Hello World")
>               .shouldHaveExitValue(0);

Addressed in the latest commit.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2091122419
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2091121979
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2091122129
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2091122658
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2091122814