On Thu, 29 Aug 2024 05:45:35 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Simon Tooke has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - simplify windwos realpath() implementation
>> - get rid of os::posix::realpath() and os::win32::realpath()
>
> This is okay in principle but a few changes can be made.
>
> Also it seems that none of the callers of `realpath` ever check `errno` so I
> think that can be removed.
>
> Thanks
Hello, @dholmes-ora , and thank you for your review! I am in the process of
attempting to address your concerns. Your review has resulted in simpler code
for my implementation - so thanks for that too. Please let me know if you have
more concerns or questions.
> src/hotspot/os/windows/os_windows.cpp line 5344:
>
>> 5342: // In this case, use the user provided buffer but at least check
>> whether _fullpath caused
>> 5343: // a memory overwrite.
>> 5344: if (errno == EINVAL) {
>
> There is nothing to indicate that `_fullpath` can ever set `EINVAL` it is
> only specified to return null on error. This code should not check errno but
> can just re-try with the user-supplied buffer.
According to [the
documentation](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fullpath-wfullpath?view=msvc-170),
EINVAL can be set in some circumstances. Those circumstances are checked
earlier in the function, so I have simplified this code.
Setting EINVAL (or ENAMETOOLONG) is part of the existing documentation for
os::realpath(); I am loathe to change it.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/20683#issuecomment-2329878249
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1744350217
