On Thu, 22 Aug 2024 18:36:39 GMT, Simon Tooke <sto...@openjdk.org> wrote:
> This PR changes the status of realpath() from a Posix-specific API to a
> globally available API, i.e. adding it to the "Hotspot Porting API". Code
> would refer to os::realpath() instead of os::Posix::realpath().
>
> This requires the addition of a stub routine in os_posix.cpp and a Windows
> implementation of realpath(), using Windows _fullpath().
>
> This PR depends on #20597 in that it removes the need for one #ifdef in that
> PR. Because of that, this PR will be modified when and if #20597 is
> integrated.
>
> Please note that guidelines for doing this appear in
> src/hotspot/share/runtime/os.hpp
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
src/hotspot/os/posix/os_posix.cpp line 896:
> 894: char* os::realpath(const char* filename, char* outbuf, size_t outbuflen)
> {
> 895: return os::Posix::realpath(filename, outbuf, outbuflen);
> 896: }
We don't need `os::Posix::realpath` any more - just rename it to `os::realpath`.
src/hotspot/os/windows/os_windows.cpp line 5319:
> 5317:
> 5318: char* os::realpath(const char* filename, char* outbuf, size_t
> outbuflen) {
> 5319: return os::win32::realpath(filename, outbuf, outbuflen);
Again you don't need the indirection here.
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.
-------------
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20683#pullrequestreview-2267709458
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1735587113
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1735588758
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1735593867
